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

Add edit form functionality #1333

Merged
merged 7 commits into from Jun 27, 2019

Conversation

Projects
None yet
4 participants
@okahilak
Copy link
Collaborator

commented Jun 11, 2019

Closes #865

@okahilak

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2019

I sent this patch to review as somewhat incomplete: I haven't written any tests yet, or necessarily updated all old tests.

However, I'd like to have a review of the general approach before putting time into writing new tests.

@okahilak okahilak force-pushed the add-edit-form branch 2 times, most recently from 7a0fb09 to f8d569f Jun 11, 2019

@okahilak okahilak force-pushed the add-edit-form branch from f8d569f to c85cfd5 Jun 12, 2019

@luontola

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

Calling the operation "create" when it's "create or update" is smelly, as is the use of a boolean flag. How about using the "update" operation to change the form fields?

When the form fields are updated, the code should first check that the form is not in use and raise an error if it is. Similar to the check when archiving a form.

@Macroz

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

When the form fields are updated, the code should first check that the form is not in use and raise an error if it is. Similar to the check when archiving a form.

Shouldn't it then notify that a new copy will be created (i.e. not an error)?

@@ -88,15 +90,22 @@
(ok (form/get-form-template form-id)))

(POST "/create" []
:summary "Create form"
:summary "Create or edit form"

This comment has been minimized.

Copy link
@Macroz

Macroz Jun 12, 2019

Collaborator

Why not have two separate endpoints? Create is when a completely new form is created and update is when one is, well, updated?

This comment has been minimized.

Copy link
@okahilak

okahilak Jun 13, 2019

Author Collaborator

You're right (as well as Esko who commented about the same): better to have the API as clean as possible. Done.

(ok (form/create-form! (getx-user-id) command)))
:body [command FormCommand]
:return FormResponse
(ok (form/create-or-edit-form! (getx-user-id) command)))

(PUT "/update" []

This comment has been minimized.

Copy link
@Macroz

Macroz Jun 12, 2019

Collaborator

There is likely a need to make sure the different kinds of update endpoints are named well. This should probably be named /update-form-state or maybe even /form/:form-id/update-state as is common in REST APIs.

This comment has been minimized.

Copy link
@okahilak

okahilak Jun 13, 2019

Author Collaborator

I started doing this but found out that there are quite a lot of places that the change touches (considering that similar update-state logic with similarly-named API end points is also implemented for catalogue items, licenses, and resources), so opted to add a TODO comment at this point and to do it in a later patch.

@okahilak

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Calling the operation "create" when it's "create or update" is smelly, as is the use of a boolean flag. How about using the "update" operation to change the form fields?

Good comments. I added another endpoint, /:form-id/edit for editing existing forms.

Using the existing "update" operation to change the form fields would have been problematic, as an identically-named operation is also in use for catalogue items, licenses, and resources, and they share the common parts. It seems therefore better to have a separate, form-specific edit operation.

I also marked as a TODO to change "update" to "update-state" in the future to avoid confusion with "edit".

When the form fields are updated, the code should first check that the form is not in use and raise an error if it is. Similar to the check when archiving a form.

Done.

@okahilak

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

When the form fields are updated, the code should first check that the form is not in use and raise an error if it is. Similar to the check when archiving a form.

Shouldn't it then notify that a new copy will be created (i.e. not an error)?

I opted to just show the error if the user attemps to edit a form that is in use, and let the user explicitly press "Copy as new" (located next to "Edit" button, after all) if that is what the user wants. I think that doing that automatically risks guessing incorrectly what the user wants to do -- for example, it could be that the user is not expecting the form to be in use and after noticing that that is the case, decides to do something else.

But this can be discussed or changed further -- I believe that having it work analogously to "Archive" button, i.e., just showing the error if the operation is non-permitted, is a decent first step.

Thanks for the good comments, I think the patch is already a lot better now after this first review round.

okahilak and others added some commits Jun 26, 2019

@hukka hukka merged commit ac64683 into master Jun 27, 2019

7 checks passed

WIP Ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details

@hukka hukka deleted the add-edit-form branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.