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

Remove free tier threshold defaults, make Plans page respect custom free tiers #4531

Merged
merged 15 commits into from Jul 12, 2023

Conversation

LMNTL
Copy link
Contributor

@LMNTL LMNTL commented Jul 6, 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 description of your work suitable for publishing on our forum
  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

Adds the ability to rename the default free plan.

Notes

The default properties of FREE_TIER_THRESHOLDS have been all set to None, since those values should be configured on a per-server basis.

The free tier on the Plans page has also been updated to reflect the values in FREE_TIER_DISPLAY.

@LMNTL LMNTL marked this pull request as ready for review July 6, 2023 21:11
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.

LGTM otherwise

@@ -27,7 +27,9 @@ import Button from 'js/components/common/button';
import classnames from 'classnames';
import LoadingSpinner from 'js/components/common/loadingSpinner';
import {notify} from 'js/utils';
import {BaseProduct} from "js/account/subscriptionStore";
import {BaseProduct} from 'js/account/subscriptionStore';
import EnvStore, {FreeTierThresholds, FreeTierDisplay} from 'js/envStore';
Copy link
Contributor

Choose a reason for hiding this comment

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

EnvStore doesn't appear to be used. If this unused import is important for some reason, it should be documented.

Comment on lines 112 to 113
const thresholds = envStore.data.free_tier_thresholds as FreeTierThresholds;
const display = envStore.data.free_tier_display as FreeTierDisplay;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the as part. Without FreeTierDisplay it will error because it seems to think it could also be {} but there would be cleaner ways to address this. Either a conditional or better yet figure out why it sometimes is {} (but the later may be out of scope)

Copy link
Contributor

Choose a reason for hiding this comment

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

I might either define it as public free_tier_display: FreeTierDisplay | null = null; if you want it to be in some null-ish state. Or else make your name optional if you consider {} to be a valid Free Tier Display.

The usage of "as" overrides type checking, making it less useful. It would be better to review how the nullish state should actually be presented.

return null;
}, [envStore.isReady]);

const hasManageableStatus = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a useCallback because you want it to be a function that accepts various subscriptions right? Just checking my own understanding, I don't think there is a change to make.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

@LMNTL LMNTL merged commit a7bd9c8 into beta Jul 12, 2023
4 checks passed
@LMNTL LMNTL deleted the free-tier-thresholds branch July 12, 2023 16:51
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