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

Fix empty proposals component configuration limits #10551

Merged
merged 2 commits into from Mar 16, 2023
Merged

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Mar 14, 2023

🎩 What? Why?

While investigating the fix for #10525, i have tried to remove all the integer field values so i could test some edge cases. This PR fixes the errors found during that test.

📌 Related Issues

Link your PR to an issue

Testing

  1. Go to proposals component config
  2. Remove all the values from integer fields, press update
  3. Visit component in the public view
  4. See it fails
  5. Apply patch, restart server to reload proposal manifest
  6. Repeat 1, 2, fill in required fields, press update
  7. Visit component in the public view
  8. See there are no errors

♥️ Thank you!

@alecslupu alecslupu added module: proposals type: fix PRs that implement a fix for a bug labels Mar 14, 2023
@alecslupu alecslupu requested a review from a team March 14, 2023 11:18
@alecslupu alecslupu added this to Pending review from Product in Maintainers via automation Mar 14, 2023
@alecslupu alecslupu marked this pull request as ready for review March 14, 2023 12:18
@alecslupu alecslupu changed the title Fix: Empty proposals config limits Fix: Empty proposals component configuration limits Mar 14, 2023
@alecslupu
Copy link
Contributor Author

alecslupu commented Mar 14, 2023

to add : proposal_edit_before_minutes
comments_max_length

@andreslucena andreslucena changed the title Fix: Empty proposals component configuration limits Fix empty proposals component configuration limits Mar 15, 2023
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these two, it works as expected.

But after searching for these I found other attributes that give the same error: the form let you save them empty and then going to the frontend page you'll see an exception.

These are the ones that I could reproduce it:

decidim-accountability/lib/decidim/accountability/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-blogs/lib/decidim/blogs/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-budgets/lib/decidim/budgets/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-debates/lib/decidim/debates/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-dev/lib/decidim/dev/test/rspec_support/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-meetings/lib/decidim/meetings/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-proposals/lib/decidim/proposals/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-sortitions/lib/decidim/sortitions/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-proposals/lib/decidim/proposals/component.rb:    settings.attribute :proposal_edit_before_minutes, type: :integer, default: 5
decidim-proposals/lib/decidim/proposals/component.rb:    settings.attribute :threshold_per_proposal, type: :integer, default: 0

Can you check them out please?

(After saving this, I saw your comment that you've found a couple of these one too, so ping me when that's implemented 😄)

@alecslupu
Copy link
Contributor Author

For these two, it works as expected.

But after searching for these I found other attributes that give the same error: the form let you save them empty and then going to the frontend page you'll see an exception.

These are the ones that I could reproduce it:

decidim-accountability/lib/decidim/accountability/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-blogs/lib/decidim/blogs/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-budgets/lib/decidim/budgets/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-debates/lib/decidim/debates/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-dev/lib/decidim/dev/test/rspec_support/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-meetings/lib/decidim/meetings/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-proposals/lib/decidim/proposals/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-sortitions/lib/decidim/sortitions/component.rb:    settings.attribute :comments_max_length, type: :integer, required: false
decidim-proposals/lib/decidim/proposals/component.rb:    settings.attribute :proposal_edit_before_minutes, type: :integer, default: 5
decidim-proposals/lib/decidim/proposals/component.rb:    settings.attribute :threshold_per_proposal, type: :integer, default: 0

Can you check them out please?

(After saving this, I saw your comment that you've found a couple of these one too, so ping me when that's implemented smile)

To be honest, i would like to address the issues of other components in a different PR.

@andreslucena
Copy link
Member

To be honest, i would like to address the issues of other components in a different PR.

OK, no problem

@alecslupu
Copy link
Contributor Author

To be honest, i would like to address the issues of other components in a different PR.

OK, no problem

Fixed here #10561

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor change

…xt.rb

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: proposals type: fix PRs that implement a fix for a bug
Projects
Development

Successfully merging this pull request may close these issues.

Deleting Boolean values from Proposal config will trigger errors
2 participants