-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[#60144] primerize wp types settings form #17704
[#60144] primerize wp types settings form #17704
Conversation
- https://community.openproject.org/work_packages/60144 - remove old form for new and edit - add primer form component
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.
Changes look (literally) nice.
(Obviously assuming the failing spec will be fixed)
- fetch correct parameter for copy workflow value on form submission - initialize form with this parameter if available - fix feature spec with new UI
979dc2a
to
af0a31d
Compare
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.
Left a couple of questions but nothing that should block the merge.
CreateTypeService | ||
.new(current_user) | ||
.call(permitted_type_params, copy_workflow_from: params[:copy_workflow_from]) do |call| | ||
.call(permitted_type_params, copy_workflow_from: params.dig(:type, :copy_workflow_from)) do |call| |
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.
🟡 Am I missing something here (most probably yes) but since we are feeding the all the permitted type params, why do we need to the copy_workflow_from
as a special ❄️?
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.
copy_workflow_from
is no property from the type record.
previously (as you can see from the change) it was passed not even inside params|type
but directly on top level params.
Usually, having something like this, I'd rework the whole form to use a form model, as this is apparently what the create service wants. But for sake of simplicity I tried to keep the previous approach alive. But with the primer forms I no longer was able to define the form input field outside the "model".
- use maybe to convert from parameter value to integer - split create and update form options - add 303 status to redirection in types controller
Ticket
OP#60144
What are you trying to accomplish?
What approach did you choose and why?