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

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

Merged
merged 12 commits into from
Aug 2, 2023

Conversation

Pezmc
Copy link
Contributor

@Pezmc Pezmc commented Aug 1, 2023

Description

Adds a new admin config option user:team:auto-create:instanceType to the admins setting screen, nested under auto-creation of team types.

If this option is set, when a user verifies their email, if they have a team (which they should always do thanks to the team:auto-create setting); the user is also given an application named Default Application and an instance, which is based on the team, and username, with a random hash, like so: team-name-username-3425aed6

Screenshot 2023-08-02 at 11 44 02
Screenshot 2023-08-02 at 11 43 57

Screen.Recording.2023-08-02.at.12.14.34.mov

Related Issue(s)

Fixes #2475

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@@ -11,6 +11,27 @@ const inflightProjectState = { }

const inflightDeploys = new Set()

class ControllerError extends Error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The long term intension would be to have a standard set of FlowForge error objects that we use throughout the app at various logical levels.

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #2553 (f19b961) into main (19f62fe) will increase coverage by 0.10%.
Report is 25 commits behind head on main.
The diff coverage is 71.82%.

@@            Coverage Diff             @@
##             main    #2553      +/-   ##
==========================================
+ Coverage   39.95%   40.06%   +0.10%     
==========================================
  Files         495      498       +3     
  Lines       17763    17865     +102     
  Branches     4145     4182      +37     
==========================================
+ Hits         7098     7158      +60     
- Misses      10665    10707      +42     
Flag Coverage Δ
backend 74.66% <89.04%> (+0.05%) ⬆️
frontend 1.57% <0.00%> (-0.01%) ⬇️

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

Files Changed Coverage Δ
forge/settings/defaults.js 100.00% <ø> (ø)
frontend/src/pages/admin/Settings/General.vue 0.00% <0.00%> (ø)
...ges/admin/TeamTypes/dialogs/TeamTypeEditDialog.vue 0.00% <0.00%> (ø)
forge/auditLog/user.js 85.07% <50.00%> (-2.23%) ⬇️
forge/routes/auth/index.js 51.13% <76.66%> (+2.37%) ⬆️
forge/db/controllers/Project.js 93.31% <90.00%> (-1.67%) ⬇️
forge/db/models/Application.js 100.00% <100.00%> (ø)
forge/db/models/Project.js 97.93% <100.00%> (+1.40%) ⬆️
forge/db/models/TeamType.js 90.56% <100.00%> (ø)
forge/routes/api/project.js 75.53% <100.00%> (-2.35%) ⬇️
... and 1 more

... and 8 files with indirect coverage changes

This method doesn't appear to be being used, and the includes clauses were totally invalid; a team doesn't have a projectype, stack or template.
@MarianRaphael MarianRaphael linked an issue Aug 2, 2023 that may be closed by this pull request
1 task
@Pezmc Pezmc force-pushed the feat-2475-auto-create-instance branch from a11f5af to e92f982 Compare August 2, 2023 09:42
@Pezmc
Copy link
Contributor Author

Pezmc commented Aug 2, 2023

There is a frustrating out of memory error holding up the tests, but otherwise this is ready for review, pinging @joepavitt for frontend and @hardillb for backend as they have context.

`app.db.models.Project.byUser(user)` seems to hand during tests, unclear why at this stage.
@Pezmc Pezmc force-pushed the feat-2475-auto-create-instance branch from f185d66 to b1a1948 Compare August 2, 2023 09:59
Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Quick read and a localfs test looks good to me,

@Pezmc Pezmc marked this pull request as ready for review August 2, 2023 10:16
@joepavitt
Copy link
Contributor

joepavitt commented Aug 2, 2023

Did try to create a new user, but hit errors in doing so. The Node-RED instance logs oddly have recordings from a few days ago?! I have also seen this issue with my other instances too, so fairly sure this is a local problem on my side.

Screenshot 2023-08-02 at 11 40 13

@joepavitt
Copy link
Contributor

Rebuilt FF, clean DB, etc. and it did create an Instance for me when signing up, however, when going to open it:

Screenshot 2023-08-02 at 11 54 06

@joepavitt
Copy link
Contributor

I also still seem to be getting logs from a different instance of Node-RED?

Screenshot 2023-08-02 at 11 54 42

@joepavitt
Copy link
Contributor

joepavitt commented Aug 2, 2023

Recommend "Default Application" being something like "Firstname Surname's Application"

Copy link
Contributor

@joepavitt joepavitt left a comment

Choose a reason for hiding this comment

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

This commit only has tests in?

@joepavitt
Copy link
Contributor

Final thought, can we know client-side if this is their first time logging in? Cookie/session storage flag/check or something maybe?

Would be good to use Alert.emit() to inform them that an instance has been created, and is being started, etc.

@Pezmc
Copy link
Contributor Author

Pezmc commented Aug 2, 2023

@joepavitt Let's investigate that as a follow up, I'm hesitant to keep increasing scope so close to release!

@Pezmc Pezmc merged commit cbed118 into main Aug 2, 2023
5 checks passed
@Pezmc Pezmc deleted the feat-2475-auto-create-instance branch August 2, 2023 11:43
@Pezmc
Copy link
Contributor Author

Pezmc commented Aug 3, 2023

For the record, I looked into how to how to track if this is the users first time landing on the page, but without implementing some sort of server side first time user experience tracking (.e.g a list of alerts the user has seen) or tracking an accounts state (again server side), there was no easy way to add it!

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.

Auto-create Application/Instance when first joining FlowForge
3 participants