Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: plan prices accurately read from Stripe #4104

Merged
merged 10 commits into from
Mar 5, 2025
Merged

Conversation

fabis94
Copy link
Contributor

@fabis94 fabis94 commented Mar 3, 2025

Description & motivation

Changes:

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

Copy link

linear bot commented Mar 3, 2025

@@ -0,0 +1,198 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
Copy link
Contributor Author

@fabis94 fabis94 Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspired by @iainsproat's recent experimentation with a generic fn/factory caching wrapper, just more decoupled & a bit easier to use - you can wrap any existing fn/factory with this without changing the fn/factory signature, so you don't need to write extra integration/resolver code

cache providers are reusable as well

see example in prices.ts or in unit tests

const store: StorageType = {
requestId: reqId,
dbMetrics: {
totalCount: 0,
totalDuration: 0
}
},
logger
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added req.logger to req context itself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 💯

Copy link
Contributor

github-actions bot commented Mar 3, 2025

📸 Preview service has generated an image.

Copy link
Contributor

github-actions bot commented Mar 3, 2025

📸 Preview service has generated an image.

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 86.61972% with 19 lines in your changes missing coverage. Please review.

Project coverage is 71.83%. Comparing base (5dbbe7f) to head (78bb919).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/server/modules/gatekeeper/stripe.ts 43.75% 8 Missing and 1 partial ⚠️
...ages/server/modules/core/graph/resolvers/common.ts 33.33% 3 Missing and 1 partial ⚠️
...kages/server/modules/gatekeeper/services/prices.ts 94.11% 1 Missing and 1 partial ⚠️
packages/server/modules/shared/utils/caching.ts 96.36% 0 Missing and 2 partials ⚠️
...ckages/server/modules/gatekeeper/clients/stripe.ts 85.71% 0 Missing and 1 partial ⚠️
...er/modules/gatekeeperCore/graph/resolvers/index.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4104      +/-   ##
==========================================
+ Coverage   71.70%   71.83%   +0.12%     
==========================================
  Files         405      410       +5     
  Lines       18342    18472     +130     
  Branches     2974     2997      +23     
==========================================
+ Hits        13152    13269     +117     
- Misses       4181     4188       +7     
- Partials     1009     1015       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@iainsproat iainsproat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible separate the rename of the existing inMemory cache, and the new caching mechanism & spec in to a separate PR?

@fabis94
Copy link
Contributor Author

fabis94 commented Mar 3, 2025

Would it be possible separate the rename of the existing inMemory cache, and the new caching mechanism & spec in to a separate PR?

@iainsproat i'd rather not, it's not a big part of the PR and its entirely new code (except a simple fn rename) so shouldn't break anything existing

Copy link
Contributor

@iainsproat iainsproat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caching part lgtm. I'll refer to someone else in backend team to review the Stripe changes.

Copy link
Contributor

github-actions bot commented Mar 3, 2025

📸 Preview service has generated an image.

@fabis94 fabis94 marked this pull request as ready for review March 3, 2025 15:48
@fabis94 fabis94 requested review from Mikehrn and gjedlicska March 3, 2025 15:48
@@ -569,7 +569,7 @@ Generate the environment variables for Speckle server and Speckle objects deploy
- name: FF_WORKSPACES_SSO_ENABLED
value: {{ .Values.featureFlags.workspacesSSOEnabled | quote }}

- name: FF_WORKSPACES_NEW_PLAN_ENABLED
- name: FF_WORKSPACES_NEW_PLANS_ENABLED
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alemagio I think u may have created a duplicate feature flag, Gergo already created the new plans FF in his initial PR

@iainsproat I left the helm chart path as is, maybe thats fine if these are supposed to be somewhat temporary

Copy link
Contributor

github-actions bot commented Mar 3, 2025

📸 Preview service has generated an image.

Mikehrn
Mikehrn previously approved these changes Mar 3, 2025
Copy link
Contributor

@Mikehrn Mikehrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, FE looks good 😁

Copy link
Contributor

github-actions bot commented Mar 3, 2025

📸 Preview service has generated an image.

@fabis94 fabis94 requested review from Mikehrn and iainsproat March 3, 2025 16:43
Mikehrn
Mikehrn previously approved these changes Mar 3, 2025
}),
getWorkspacePlanProductAndPriceIds
})
const prices = await getWorkspacePlanPrices.fresh()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, left this on .fresh() (skips cache) for testing, i'll switch it back to the cached version when the PR is ready to go

@fabis94 fabis94 requested review from cdriesler and alemagio March 4, 2025 08:33
alemagio
alemagio previously approved these changes Mar 5, 2025
@fabis94 fabis94 dismissed stale reviews from alemagio and Mikehrn via 78bb919 March 5, 2025 10:04
Copy link
Contributor

github-actions bot commented Mar 5, 2025

📸 Preview service has generated an image.

@fabis94 fabis94 merged commit 954b1a9 into main Mar 5, 2025
30 of 33 checks passed
@fabis94 fabis94 deleted the fabians/web-2738 branch March 5, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants