Add support for multiple TeamTypes with limits/billing set per type #2519

knolleary · 2023-07-24T11:25:14Z

Description

Part of #2358

This expands the TeamTypes model to support the ability to have different TeamTypes with different limits/billing applied.

It is a significant piece of work as it required a lot of changes to how billing is structured given the configuration now has to come based on the TeamType.

I have structured the commits to try to make a bit more reviewable.

DB Changes

TeamType - renames 'enabled' to 'active' to be consistent with 'ProjectTypes'
TeamType - adds 'order' column so the types can be listed in the desired order in the UI

The Team model has had a lot of instance functions added to it to access information about the team. This is a much cleaner approach. When running EE with billing, some of those functions are overloaded to add billing-specific checks and logic. This has removed the need for a lot of billing checks in the base CE code.

API Changes

/api/v1/team-types/*** - apis added to create/update/list/delete team types

Config changes

Currently all team pricing config is provided in the yml file. With this PR, that info can now be set by admin in the UI.

When creating a TeamType, the admin has to provide a lot of information:

Stripe Price/Product for the TeamType itself
For each InstanceType on the platform - whether it is available to that Team, any limit on how many, a 'free allowance', and stripe price/product info
Device price/product
User limits (if any)
Trial mode options
Feature flags - although these are not currently honoured.

When the platform starts, it checks for the existing default 'starter' TeamType and adds if it necessary. If billing is enabled, it also adds the new Starter and Team (previously called Premium...) types - but leaves then inactive so the platform admin can choose when to enabled them (once they have finished configuring them).

Trial mode

Previously, trial mode was configured in admin settings - setting duration and what InstanceType is available. That configuration has moved to the individual TeamTypes configuration.

In Admin settings, if the 'create personal team' option is enabled, there is now a choice of what TeamType that personal team should be. It will default to the existing 'starter' team until configured otherwise.

What is missing before we can fully launch the new tiers in FF Cloud:

Upgrade path. There is currently no way for a team to change its type.
Feature limits - the UI lets the admin tick boxes to say what features should be enabled for a given type, but currently there are no restrictions applied
UX work around team creation/upgrade/billing. We have designs, but they are not yet implemented (and couldn't be until this base work was done).

codecov · 2023-07-24T11:35:35Z

Codecov Report

Merging #2519 (fe4a9db) into main (65703f1) will decrease coverage by 0.37%.
Report is 4 commits behind head on main.
The diff coverage is 47.52%.

@@            Coverage Diff             @@
##             main    #2519      +/-   ##
==========================================
- Coverage   40.20%   39.83%   -0.37%     
==========================================
  Files         491      494       +3     
  Lines       17319    17690     +371     
  Branches     4020     4125     +105     
==========================================
+ Hits         6963     7047      +84     
- Misses      10356    10643     +287

Flag	Coverage Δ
backend	`74.47% <81.07%> (-0.16%)`	⬇️
frontend	`1.59% <0.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
forge/db/index.js	`60.78% <ø> (-7.08%)`	⬇️
...igrations/20220808-02-create-default-team-types.js	`0.00% <ø> (-88.89%)`	⬇️
...orge/db/migrations/20230711-01-update-team-type.js	`0.00% <0.00%> (ø)`
forge/db/models/Application.js	`100.00% <ø> (ø)`
forge/db/models/Device.js	`76.19% <ø> (ø)`
forge/db/models/Project.js	`96.52% <ø> (ø)`
forge/db/models/ProjectSettings.js	`100.00% <ø> (ø)`
forge/db/views/Team.js	`86.36% <ø> (ø)`
.../ee/lib/billing/emailTemplates/TrialTeamCreated.js	`100.00% <ø> (ø)`
...ge/ee/lib/billing/emailTemplates/TrialTeamEnded.js	`100.00% <ø> (ø)`
... and 36 more

... and 3 files with indirect coverage changes

knolleary · 2023-07-24T14:41:02Z

NOw investigating when postgres hits failures that sqlite doesn't. Not immediately obvious the difference.

knolleary · 2023-07-24T15:12:04Z

Postgres tests should be fixed with the most recent commit. The tests were doing a TeamType.findOne() to get the default team type - which is fine when there is only one. However, if billing is enabled, there will now be 3 team types - old starter, new Starter and new Premium. So the tests needed to be updated to ensure they got the old starter type.

Pezmc · 2023-07-24T19:31:53Z

forge/containers/wrapper.js

-        }
+        // This will perform all checks needed to ensure this instance
+        // can be started - throws err if not
+        await project.Team.checkInstanceStartAllowed(project)


That this needs a comment to explain how the method works each time feels like a code smell to me.

How about naming it ensureInstanceStartAllowed, or throwIfInstanceStartNotAllowed?

…model

knolleary · 2023-07-26T09:38:23Z

I have just rebased to main and force pushed. This ensures the migration files are all present and correct in this branch following the merge of the pipeline work.

Pezmc

Comments thus far, am about half way through a re-review. Just eating lunch!

Pezmc · 2023-07-26T11:15:46Z

forge/db/views/TeamType.js

            description: teamType.description,
            properties
        }
+        if (includeAdminOnlyProps && teamType.get) {
+            // For some API calls, the teamType passed here is a raw Object, not


Can we change this so it's always a teamType object? Why do we need to support both?

I agree this isn't ideal. I can't remember the specific code path that lead to needing to do this, but it was sufficiently non-trivial to resolve quickly that I worked around it.

Pezmc · 2023-07-26T11:16:27Z

forge/routes/api/teamType.js

+            filter = { active: false }
+        }
+        const teamTypes = await app.db.models.TeamType.getAll(paginationOptions, filter)
+        teamTypes.types = teamTypes.types.map(pt => app.db.views.TeamType.teamType(pt, request.session.User.admin))


What's a PT?

A remnant of when I copy and pasted the boilerplate of a CRUD api from the ProjectTypes api.

Pezmc · 2023-07-26T11:18:28Z

forge/db/models/Team.js

                    const where = { TeamId: this.id }
                    if (projectTypeId) {
                        if (typeof projectTypeId === 'string') {
                            projectTypeId = M.ProjectType.decodeHashid(projectTypeId)
+                        } else if (projectTypeId.id) {


I think this should throw rather than silently fixing the error, that's bound to lead to confusion down the line. Is there a reason we need to support calling instanceCount passing in an object?

Pezmc · 2023-07-26T11:20:29Z

forge/db/models/Team.js

+                 * Depending on the route taken, it is possible this property has not
+                 * been fully loaded. This does the work to ensure it is there if needed.
+                 */
+                ensureTeamTypeExists: async function () {


I think I'm not clear on why everywhere wouldn't just use sequelizes await this.getTeamType() in all places, rather than having this logic to check whether or not TeamType was included when the team model was loaded.

See comment below on why handling TeamTypeId being undefined is needed - if not ideal.

Pezmc · 2023-07-26T11:20:52Z

forge/db/models/Team.js

+                 */
+                ensureTeamTypeExists: async function () {
+                    if (!this.TeamTypeId) {
+                        await this.reload({ include: [{ model: M.TeamType }] })


If TeamTypeId is undefined, why would reloading the Team model make a difference here? Is there somewhere we only load the models but with not all the fields?

There were multiple places where we load a model and include Team, but limit it to the minimum of team attributes that are needed in that context. That would mean it didn't have TeamTypeId because it wasn't one of the selected attributes.

The reload ensures the full Team object is retrieved and the TeamType can be included.

I did go through a lot of the relevant models to ensure they included TypeTypeId, but wasn't 100% confident there wasn't still a reload lurking somewhere that would get missed. This is a safety net just in case.

Pezmc · 2023-07-26T11:24:28Z

forge/db/models/Team.js

+                 * properties set
+                 * @param {object} instanceType - a fully populated ProjectType object
+                 */
+                checkInstanceTypeCreateAllowed: async function (instanceType) {


I think I dropped this on the previous PR, but I'd expect a function called check to return a boolean, rather than throwing an error.

Naming this something more explicit like: raiseIfInstanceTypeCreateNotAllowed or requireInstanceTypeCreateAllowed gives a better clue that it works by raising exceptions.

Pezmc · 2023-07-26T12:29:16Z

forge/routes/api/device.js

-        const teamDeviceLimit = team.TeamType.getProperty('deviceLimit')
-        if (typeof teamDeviceLimit === 'number') {
+        const teamDeviceLimit = await team.getDeviceLimit()
+        if (teamDeviceLimit > -1) {


Personal preference >= 0 (though is 0 valid?)

Pezmc · 2023-07-26T12:34:14Z

forge/routes/api/projectActions.js

-                        return
-                    }
+                try {
+                    // This will perform all checks needed to ensure this instance


Re-reading these comments explaining what the function does is definitely a code smell, a clearer name should make them redundant!

Pezmc · 2023-07-26T12:50:37Z

frontend/src/pages/admin/Settings/General.vue

+                label: tt.name
+            }
+        })
+        this.teamTypes.sort((A, B) => { return A.order - B.order })


Usually this would go in a computed function, but we're not mutating teamTypes at any point, so this is ok for now.

Pezmc

This was a long PR. Nick and I have spoken on slack about some of the nit-picks and follow ups to improve readability down the line, but no functionality changes are needed; so we're happy for this to be merged (for testing on staging) and them addressed as follow ups.

Pezmc · 2023-08-02T09:40:49Z

I hit a few edge cases with this while testing #2553, all centered around adding a new instanceType after the team type has been created.

Fixes and test coverage are in 49efe7e and 40f6507, but we should take extra care to verify editing team types before and after adding new instanceTypes on staging.

/CC: @MarianRaphael

Changes were introduced in #2519

knolleary requested a review from Pezmc July 24, 2023 11:25

Pezmc reviewed Jul 24, 2023

View reviewed changes

knolleary mentioned this pull request Jul 25, 2023

Auto-create Application/Instance when first joining FlowForge #2475

Closed

1 task

knolleary added 17 commits July 26, 2023 09:56

Update TeamType table - rename enabled to active and add order col

57dcdf6

Ensure default team types are present and correct

ca49c19

Update Team/Type views

ae12807

Add instance functions to Team model in both CE and EE modes

5f931e3

Update other models to use Team instance functions

ef5a656

Add Admin UI for managing TeamTypes

7593122

Add TeamType route permissions

adc8603

Update platform settings for team trial mode selection

8c93580

Update all billing logic to work with TeamTypes

41de7be

Update team trial task logic to handle trial expiry with new billing …

ba91da7

…model

Update billing ux to show correct instance pricing based on team type

2b2551c

Update all the tests for new teamtype/billing/trials logic

5b77576

Update stripeMock to not mutate passed in objects

f90f24f

Update tests to work with mocked stripe api

da7dc32

Ensure all tests pick the right default team type

35c1dc0

Ensure consistent feature flag naming in teamtype

450eeb9

Ensure we don't try to remove a subscription item that does not exist

36c2439

knolleary force-pushed the 2359-teamtypes-api branch from 1dcc7a1 to 36c2439 Compare July 26, 2023 09:37

knolleary added 2 commits July 26, 2023 11:08

Revert billing change

53b44c7

Add timeout to gh runner and increase mocha timeout

fe4a9db

Pezmc suggested changes Jul 26, 2023

View reviewed changes

Pezmc reviewed Jul 26, 2023

View reviewed changes

Pezmc approved these changes Jul 26, 2023

View reviewed changes

knolleary merged commit 8da2139 into main Jul 26, 2023
4 of 5 checks passed

knolleary deleted the 2359-teamtypes-api branch July 26, 2023 12:56

This was referenced Jul 26, 2023

TeamTypes: enable feature flag constraints #2531

Closed

Introduction of Pre-Paid Resource Packages per Team #2359

Closed

Pezmc added a commit that referenced this pull request Oct 19, 2023

Correct the test to reflect latest billing changes

40d6c45

Changes were introduced in #2519

Pezmc mentioned this pull request Oct 19, 2023

Chore: Throw for unhandled promise rejections in tests #2973

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple TeamTypes with limits/billing set per type #2519

Add support for multiple TeamTypes with limits/billing set per type #2519

knolleary commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

knolleary commented Jul 24, 2023

knolleary commented Jul 24, 2023

Pezmc Jul 24, 2023

knolleary commented Jul 26, 2023

Pezmc left a comment

Pezmc Jul 26, 2023

knolleary Jul 26, 2023

Pezmc Jul 26, 2023

knolleary Jul 26, 2023

Pezmc Jul 26, 2023

Pezmc Jul 26, 2023

knolleary Jul 26, 2023

Pezmc Jul 26, 2023

knolleary Jul 26, 2023

Pezmc Jul 26, 2023

Pezmc Jul 26, 2023

Pezmc Jul 26, 2023

Pezmc Jul 26, 2023

Pezmc left a comment

Pezmc commented Aug 2, 2023

Add support for multiple TeamTypes with limits/billing set per type #2519

Add support for multiple TeamTypes with limits/billing set per type #2519

Conversation

knolleary commented Jul 24, 2023 • edited Loading

Description

DB Changes

API Changes

Config changes

Trial mode

codecov bot commented Jul 24, 2023 • edited Loading

Codecov Report

knolleary commented Jul 24, 2023

knolleary commented Jul 24, 2023

Choose a reason for hiding this comment

knolleary commented Jul 26, 2023

Pezmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pezmc left a comment

Choose a reason for hiding this comment

Pezmc commented Aug 2, 2023

knolleary commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading