[60869] No error message shows on Relations modal if no WP is selected #17776
Conversation
HDinger
force-pushed
the
bug/60869-no-error-message-shows-on-relations-modal-if-no-wp-is-selected
branch
from
8efb664 to
614869a
Compare
| # in this case we only want to show the "WorkPackage can't be blank" error instead of a | ||
| # misleading circular dependencies error | ||
| # the error is added by the models presence validation | ||
| return if model.to_id.nil? |
There was a problem hiding this comment.
This is probably the most critical part of that PR. Can that have unwanted side effects?
There was a problem hiding this comment.
I think this change is fine. The part I would be most concerned about is that a relation might be created without a to_id being set. But this is prevented by the other validation validate_to_exists.
But what is missing here is an equivalent check for from_id. A relation that is created will not always take to_id to be the work package the user selected. That is because the relations are normalized where possible. As an example, the two relations 'blocks' and 'blocked_by' are normalized to blocks. To do so, from_id and to_id are switched for 'blocked_by', so that a 'blocks' can then be saved.
That is the reason why submitting the form with an empty work package input for a 'Blocked by' relation causes an error.
Be warned: The whole frontend is currently working with the assumption that to will always be the 'other' work package so I would expect other errors to follow.
| @@ -59,6 +59,15 @@ def create | |||
| service_result = Relations::CreateService.new(user: current_user) | |||
| .call(create_relation_params) | |||
|
|
|||
| unless service_result.success? | |||
There was a problem hiding this comment.
💡 You can use if service_result.failure? here which to me reads more easily
| # in this case we only want to show the "WorkPackage can't be blank" error instead of a | ||
| # misleading circular dependencies error | ||
| # the error is added by the models presence validation | ||
| return if model.to_id.nil? |
There was a problem hiding this comment.
I think this change is fine. The part I would be most concerned about is that a relation might be created without a to_id being set. But this is prevented by the other validation validate_to_exists.
But what is missing here is an equivalent check for from_id. A relation that is created will not always take to_id to be the work package the user selected. That is because the relations are normalized where possible. As an example, the two relations 'blocks' and 'blocked_by' are normalized to blocks. To do so, from_id and to_id are switched for 'blocked_by', so that a 'blocks' can then be saved.
That is the reason why submitting the form with an empty work package input for a 'Blocked by' relation causes an error.
Be warned: The whole frontend is currently working with the assumption that to will always be the 'other' work package so I would expect other errors to follow.
| @@ -105,7 +105,8 @@ class Relation < ApplicationRecord | |||
|
|
|||
| validates :lag, numericality: { allow_nil: true } | |||
|
|
|||
| validates :to, uniqueness: { scope: :from } | |||
| # The primer form depends on to_id being validated | |||
| validates :to_id, uniqueness: { scope: :from }, presence: true | |||
There was a problem hiding this comment.
This would not be necessary if to was used in the form. The contract already adds a validation error in this case (which admittedly reads plain bad). So this is currently duplicating functionality.
The best solution would be one where errors added to something (like to) would also be picked up by fields defined for something_id (like to_id). But we don't have such a thing in place.
Next best thing in my book would be to check if the contract can be changed to work on to_id and from_id without breaking the API.
But with the need of the frontend to also work with from which might require some extra work either in the controller or the component, new opportunities for handling the error messages correctly might also open up.
Ticket
https://community.openproject.org/projects/openproject/work_packages/60869/activity
What are you trying to accomplish?
Show an error when trying to submit the relation dialog without having selected a WorkPackage.
Screenshots