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

Improve error messages in admin panel #8193

Merged
merged 7 commits into from
Sep 6, 2021

Conversation

entantoencuanto
Copy link
Contributor

@entantoencuanto entantoencuanto commented Jul 9, 2021

🎩 What? Why?

This PR:

  • Adds a Presenter to generate a error message associated with a form with validations failed in html format including the list of validation errors
  • Allows partial which displays flash messages in admin to accept a html argument and render it
  • Changes error message described in Error merging proposals #5825 to use html and use a more meaningful description of the error

📌 Related Issues

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Before:

Screen Shot 2021-07-09 at 18 24 43

After:

Screen Shot 2021-07-20 at 21 57 26

♥️ Thank you!

@entantoencuanto entantoencuanto force-pushed the feature/improve_error_messages_in_admin_panel branch from 4041682 to 492c949 Compare July 9, 2021 15:42
@entantoencuanto entantoencuanto marked this pull request as ready for review July 9, 2021 16:51
@andreslucena andreslucena changed the title Feature/improve error messages in admin panel Improve error messages in admin panel Jul 13, 2021
@leio10
Copy link
Contributor

leio10 commented Sep 1, 2021

@decidim/product is this PR ready to be reviewed?

@andreslucena
Copy link
Member

@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.

leio10
leio10 previously approved these changes Sep 1, 2021
Copy link
Contributor

@leio10 leio10 left a 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
Copy link
Contributor

@leio10 leio10 Sep 1, 2021

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@leio10 leio10 merged commit b668e6a into develop Sep 6, 2021
@leio10 leio10 deleted the feature/improve_error_messages_in_admin_panel branch September 6, 2021 09:08
leio10 added a commit that referenced this pull request Sep 6, 2021
@leio10 leio10 mentioned this pull request Sep 6, 2021
12 tasks
leio10 added a commit that referenced this pull request Sep 6, 2021
@andreslucena andreslucena added type: feature PRs or issues that implement a new feature and removed type: enhancement labels Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review module: admin module: proposals type: feature PRs or issues that implement a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review errors in admin panel
4 participants