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

Show storage add-ons in usage limits #4660

Merged
merged 17 commits into from Oct 12, 2023
Merged

Conversation

LMNTL
Copy link
Contributor

@LMNTL LMNTL commented Sep 26, 2023

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

Previously purchase storage add-ons are now used in usage limit calculations. Corrects some issues that were causing incorrect limits to be displayed.

Notes

We previously determined whether to show the FREE_TIER_THRESHOLDS based on the logged-in user's join date. But for multi-user organizations, that meant each user in the organization could potentially see a different set of limits for their organization. This PR changes that so custom tiers are only shown if the organization owner joined before FREE_TIER_CUTOFF_DATE. If the user isn't in an organization, it still defaults to checking the user's join date.

@LMNTL LMNTL changed the title Storage addons frontend Show storage add-ons in usage limits Sep 27, 2023
@LMNTL LMNTL marked this pull request as ready for review September 27, 2023 15:54
@LMNTL LMNTL requested a review from bufke September 28, 2023 14:17
Copy link
Contributor

@bufke bufke left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of unit tests for JS, but I feel that getLimitsForMetadata and others have gotten too complex and merits it. Doesn't need to be now and happy to discuss. Tightening up the types (or simplifying) would mitigate my concern. It feels like the business logic has just gotten a bit too complex from where it started. It's not longer obvious at a glance as to what it's doing.

jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
jsapp/js/account/stripe.api.ts Show resolved Hide resolved
kpi/views/environment.py Outdated Show resolved Hide resolved
@LMNTL LMNTL requested a review from bufke October 2, 2023 20:58
Copy link
Contributor

@bufke bufke left a comment

Choose a reason for hiding this comment

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

We discussed that the Environment API be better off not being user specific but that might be out of scope.

kpi/views/environment.py Outdated Show resolved Hide resolved
@@ -4,3 +4,15 @@
'past_due',
'trialing',
]

FREE_TIER_NO_THRESHOLDS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly out of scope. If there was ever a good case for type hints, this is it. Lacking type hints, a DRF serializer on the Environments endpoint would help ensure correctness. But the environments endpoint doesn't have one. IMO this should be considered in a refactor. You could ponder if the added complexity here merits adding a serializer. While not as powerful as real type hints, the serializer would provide a runtime guarantee that data is returned in the expected format. Currently it's a untyped dict that is returned, which cannot provide proof that the API consistently returns the expected format the Frontend needs to accept.

@LMNTL LMNTL merged commit 5ed6a45 into release/2.023.37 Oct 12, 2023
4 checks passed
@LMNTL LMNTL deleted the storage-addons-frontend branch October 12, 2023 16:07
@notion-workspace
Copy link

@p2edwards p2edwards mentioned this pull request Oct 17, 2023
7 tasks
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.

None yet

2 participants