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: allow 2 custom templates for every user #9518

Merged
merged 15 commits into from
Mar 22, 2024

Conversation

nickoferrall
Copy link
Contributor

@nickoferrall nickoferrall commented Mar 8, 2024

Fix #9473

Demo: https://www.loom.com/share/122c8e9e55db443cb52908a1a6957c07

To test

  • A user on the Starter tier can create two custom Retro templates and they'll see the upgrade CTA on the third attempt
  • Same goes for Poker
  • If they're on the Team or Enterprise tier, they can create as many custom templates as they like

@github-actions github-actions bot added the size/m label Mar 8, 2024
@nickoferrall nickoferrall marked this pull request as ready for review March 11, 2024 14:14
featureFlags {
noTemplateLimit
}
freeCustomRetroTemplatesRemaining
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created two flags, freeCustomRetroTemplatesRemaining and freeCustomPokerTemplatesRemaining, thinking of our engineering principle that we should repeat the code twice and, on the third time, refactor it into something reusable.

@@ -15,6 +17,14 @@ const AddPokerTemplatePayload = new GraphQLObjectType<any, GQLContext>({
if (!templateId) return null
return dataLoader.get('meetingTemplates').load(templateId)
}
},
user: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll refactor this to the SDL pattern in a follow-up PR. This PR already touches a lot of files, so I didn't want to bloat it further

@@ -53,16 +53,17 @@ const AddNewPokerTemplate = (props: Props) => {
id
user {
id
featureFlags {
noTemplateLimit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we've removed the template limit, we should remove this flag altogether. I've created an issue: #9523

packages/server/graphql/mutations/addPokerTemplate.ts Outdated Show resolved Hide resolved
packages/server/graphql/public/types/User.ts Outdated Show resolved Hide resolved
import {decrementFreeRetroTemplatesRemainingQuery} from './generated/decrementFreeCustomRetroTemplatesRemainingQuery'
import {decrementFreePokerTemplatesRemainingQuery} from './generated/decrementFreeCustomPokerTemplatesRemainingQuery'

const decrementFreeTemplatesRemaining = async (userId: string, templateType: 'retro' | 'poker') => {
Copy link
Member

Choose a reason for hiding this comment

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

-1 pg-typed is deprecated. can we use kysely instead here?

Copy link
Member

Choose a reason for hiding this comment

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

@nickoferrall thoughts on changing this to kysely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I've updated the migration to kysely, but not the query. I can do this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@Dschoordsch
Copy link
Contributor

Please fix the PR title as it will appear in the changelog

@nickoferrall
Copy link
Contributor Author

Please fix the PR title as it will appear in the changelog

@Dschoordsch it passes the PR title check. What would you like me to change it to?

@Dschoordsch
Copy link
Contributor

Please fix the PR title as it will appear in the changelog

@Dschoordsch it passes the PR title check. What would you like me to change it to?

Yes, it passes the check, but if you read

Added

  • two custom template limit

would you know what it means?

How about "Allow 2 custom templates for every user"?

@nickoferrall nickoferrall changed the title feat: two custom template limit feat: allow 2 custom templates for every user Mar 13, 2024
? 'freeCustomRetroTemplatesRemaining'
: 'freeCustomPokerTemplatesRemaining'

const userBeforeUpdate = await pg
Copy link
Member

Choose a reason for hiding this comment

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

-1 this is slightly worse than the SQL you had because if this function gets called twice then we risk the first query being a stale read. If there's ever a way to do something in a single SQL command, that should be the goal!

See Playground Link

const person = await db
  .updateTable('User')
  .set((eb) => ({[field]: eb(field, '-', '1')} ))
  .where('id', '=', userId)
  .where(field, '>', 0)
  .executeTakeFirst() 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks! Updated

@mattkrick mattkrick merged commit 2352669 into master Mar 22, 2024
6 checks passed
@mattkrick mattkrick deleted the feat/9473/two-custom-template-limit branch March 22, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit Starter tier users to 2 custom templates
3 participants