-
Notifications
You must be signed in to change notification settings - Fork 60
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
Change Project Type Form #1610
Change Project Type Form #1610
Conversation
This splits the _billing_ related logic into a separate component And all the other logic into a ProjectForm
a24f1af
to
74fbc10
Compare
@Pezmc just flagging a conflict, fairly sure we want what you have, but wanted to make sure as it's likely linked to the moving of the "you have credit" message |
forge/routes/api/project.js
Outdated
@@ -361,7 +361,55 @@ module.exports = async function (app) { | |||
} | |||
}, async (request, reply) => { | |||
let changed = false | |||
if (request.body.stack) { | |||
if (request.body.changeProjectType === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This end point is somewhat overloaded, but it does already let you set the ProjectType of the project only if the project doesn't already have a ProjectType set. This was added when we introduced ProjectTypes as a one-off task for users to set the type of their project. This is why the handling for that will bail out early if the Project already has a ProjectType set.
I'm wondering if, rather than introduce a changeProjectType
flag when we don't have equivalents for the other things you can change, this logic should be moved down to the existing handling when body.projectType
is set (line 434 of the proposed changes).
That block then has two branches - one for setting ProjectType initially, and one for changing ProjectType.
We should look at the data on production, but handling projects without a ProjectType is very much a legacy edge case that would only apply to FF instances that have been running since before ProjectTypes were introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if, rather than introduce a changeProjectType flag when we don't have equivalents for the other things you can change, this logic should be moved down to the existing handling when body.projectType is set (line 434 of the proposed changes).
Essentially, there was the original "no project type" code that was rather implicit in the fact that if body
supplied a projectType
.
With the introduction of Change Project Type, the body
needs to contain both projectType
AND stack
in the body
- so the explicit changeProjectType
was introduced to make this 100% explicit as to intention rather than inferring intention from values in body. It is much more deliberate than being present or not.
Also, since the first part of the logic would capture execution (because body.stack
is needed in a project type change) we would have to have some if (stack && !projectType)
logic in the first part of the handler (i.e. less explicit intention)
I've resolved the conflict with main from #1613 and made a couple of minor tweaks to make the endpoint easier to follow. I'm going to re-work that method in a follow up PR as it's become incredibly overloaded. |
Can you summarize the public API changes this is making? (ie to the |
@knolleary The only API change is adding a new flag |
125a4ba
to
bcd0487
Compare
For reference, the refactored method can be found in https://github.com/flowforge/flowforge/blob/125a4ba5d556797e37b0c6b6f6f8d604723c179c/forge/routes/api/project.js#L346-L595 |
Having tried this out, I see you've combined changing stack and changing type into one option. Updating the stack is a far more routine maintenance action on the project than changing the type. That said, we did introduce versioning of stacks - and if there is a new version of the existing stack available, they get an 'Update Stack' button which gives them a one-click update without having to make any choices: For the new button 'Change Project Settings' doesn't feel like the right text. As we discussed in slack, Project Settings means something else here. How about just labelling it as |
Hi @knolleary not sure if you are asking if the "Update Stack" is now part of the "Change Project Type" or if you are answering a previous question - but for reference, the "Update Stack" button is still there on the form: https://github.com/flowforge/flowforge/pull/1610/files#diff-a5605eb4d14a272712bacb8569e913d4c3f1885a374d594f77277d7d91d2656bR20-R29 Apologies if I am misreading. |
I appreciate my comment is a little muddled because I changed tack half way through writing it. My original point is the 'Change Stack' button has been replaced by this new 'Change Project Settings' button This means changing a project's stack now forces the user through billing considerations, whereas it is only when they change the type will anything need to change with their billing. |
@knolleary I've addressed the comments from our call earlier this morning and this PR is ready for re-review Danger Screen
Change Project Page
Screenshots updated in description |
4a48055
to
53c1c55
Compare
Description
Implements changing of project types as a "danger" action (this section likely to be renamed in future).
When a user changes project type, the project is suspended, reconfigured, then resumed.
Technically, this uses the same form as the create project form, with some tweaks in behaviour such as hiding the template fields.
UI After UX Review
See comment thread for details.
Original UI from PR
Related Issue(s)
#595
Checklist
Requirements