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 showing not visible proposals in process home page #3175

Merged
merged 4 commits into from
Apr 11, 2018

Conversation

rbngzlv
Copy link
Contributor

@rbngzlv rbngzlv commented Apr 9, 2018

🎩 What? Why?

The detail of the process shows all the proposals of that process. It doesn't filter if the proposals are draft, moderated or withdrawn.

📌 Related Issues

📌 Related PR

📋 Subtasks

  • Add CHANGELOG entry

📷 Screenshots (optional)

@ghost ghost assigned rbngzlv Apr 9, 2018
@ghost ghost added the status: WIP label Apr 9, 2018
@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 9, 2018

@decidim/lot-core and @deivid-rodriguez can you take a look on specs to check if is possible to write it in a better way? I would appreciate it very much.

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Apr 9, 2018

@rbngzlv Good job. The specs look perfectly readable to me!

One thing I think could be improved is those magic numbers: the hardcoded number of hightlighted proposals the view hooks display. If we extract those numbers to a class methods, constant, config_accessors or whatever, we can mock those methods in the spec, making it more readable, removing the need for the comments, and making the spec resilient to a change in the magic number.

@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 10, 2018

One thing I think could be improved is those magic numbers: the hardcoded number of hightlighted proposals the view hooks display. If we extract those numbers to a class methods, constant, config_accessors or whatever, we can mock those methods in the spec, making it more readable, removing the need for the comments, and making the spec resilient to a change in the magic number.

This is just what I was asking, how to improve the magic numbers. Thanks @deivid-rodriguez!

@rbngzlv rbngzlv force-pushed the fix/proposal_drafts_in_process_page branch from d9a8b5f to a926973 Compare April 10, 2018 13:09
@rbngzlv rbngzlv force-pushed the fix/proposal_drafts_in_process_page branch from a926973 to 651c2bf Compare April 10, 2018 13:19
@rbngzlv rbngzlv force-pushed the fix/proposal_drafts_in_process_page branch from 651c2bf to 68d91a8 Compare April 10, 2018 13:22
@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 10, 2018

@decidim/lot-core this PR fixing a bug is ready to be reviewed and merged. Once merged I'll do a backport to 0.10-stable.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Couple of minor comments, but looks good to me otherwise!

CHANGELOG.md Outdated
@@ -10,6 +10,8 @@ controller or added a new module you need to rename `feature` to `component`.

**Added**:

- **decidim-proposals**: Fix view hooks returning proposals that should not be shown [\#3175](https://github.com/decidim/decidim/pull/3175)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go to the bug fixes section.

let!(:proposals) { create_list(:proposal, highlighted_proposals + 2, component: component) }

it "shows the amount of proposals configured" do
visit resource_locator(participatory_process).path
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the participatory_process_group_path where you want to go here?

@ghost ghost added the status: WIP label Apr 10, 2018
@rbngzlv
Copy link
Contributor Author

rbngzlv commented Apr 10, 2018

Thanks for the review @deivid-rodriguez, your comments are totally true. Fixed!

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks good to me!

# Public Setting that defines how many proposals will be shown in the
# process_group_highlighted_elements view hook
config_accessor :process_group_highlighted_proposals_limit do
3
Copy link
Contributor

Choose a reason for hiding this comment

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

These two new settings have different values, is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! It is intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants