-
-
Notifications
You must be signed in to change notification settings - Fork 413
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
Improve error messages in admin panel #8193
Conversation
4041682
to
492c949
Compare
@decidim/product is this PR ready to be reviewed? |
Yes, it's OK for us. At least this improves a bit the error message in the merging error case. |
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.
Nice job! 👍
@entantoencuanto I'll wait for your opinion about my comment before merging this.
@@ -37,7 +37,11 @@ def mergeable_to_same_component | |||
!proposal.official? || proposal.votes.any? || proposal.endorsements.any? | |||
end | |||
|
|||
errors.add(:proposal_ids, :public_proposals) if public_proposals | |||
if public_proposals |
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.
💬 Would it be clear for the user if you do the add the errors inside the previous loop depending on the detected problems?
You can use a set to avoid adding the same error twice, like this:
errors_set = Set[]
proposals.each do |proposal|
errors_set << :published if proposal.published? # this was not checked before, I don't know the exact logic
errors_set << :official if proposal.official?
errors_set << :supported if proposal.votes.any? || proposal.endorsements.any?
end
errors_set.each {|error| errors.add(:base, error)} if errors_set.any?
Of course, in the case you use this approach, you should also change the message a bit.
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.
Good point, I'll make the changes you suggest, I'll ping you when ready
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've updated the messages and removed the published? condition, I've been looking for a reason for this condition and I haven't found anything. I've also inverted the validation message about official proposals.
🎩 What? Why?
This PR:
📌 Related Issues
Testing
Describe the best way to test or validate your PR.
📋 Checklist
🚨 Please review the guidelines for contributing to this repository.
docs/
.📷 Screenshots
Before:
After: