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

Show link to evaluate investments with valuation finished #5078

Merged
merged 5 commits into from
Feb 20, 2023

Conversation

javierm
Copy link
Member

@javierm javierm commented Feb 6, 2023

References

Objectives

  • Make it easy to access the "Evaluate" section when there aren't investments under valuation but there are investments whose valuation has finished
  • Simplify the code loading investments in the valuation section

@javierm javierm added Bug Coadministration Management, Moderation and Valuation labels Feb 6, 2023
@javierm javierm requested a review from Senen February 6, 2023 13:33
@javierm javierm self-assigned this Feb 6, 2023
@javierm javierm added this to Reviewing in Consul Democracy Feb 6, 2023
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Feb 6, 2023
@javierm javierm moved this from Doing to Reviewing in Consul Democracy Feb 6, 2023
@javierm javierm marked this pull request as draft February 6, 2023 13:36
Copy link
Member

@Senen Senen left a comment

Choose a reason for hiding this comment

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

Hi @javier, I'm okay with all the changes. I think this change should cover the initially intended behaviour.

We're going to add more cases and the test for the link to the evaluate
would become too big, so we're splitting it.
This way it's still possible to access the "evaluation finished" filter
in the valuation investments index.
Using this method makes it more obvious that we're loading the same
investments in the budgets index as in the investments index.
We weren't allowing the `budget_id` parameter and then we were adding it
manually. We were also allowing other parameters that aren't used in the
valuation section.

So we're allowing budget and heading, which are the only parameter we're
offering filters for in the user interface. Note the `budget_id`
parameter doesn't seem to make sense because we're already inside a
`@budget.investments` statement, but the `budget_id` parameter is
required by the `scoped_filter` method.
The budget is loaded using a method which raises an exception if it
isn't found, so `@budget.present?` will always return true.
@javierm javierm marked this pull request as ready for review February 17, 2023 14:40
@javierm
Copy link
Member Author

javierm commented Feb 17, 2023

@Senen Cool! I've reorganized the pull request and rebased it against the latest master.

Consul Democracy automation moved this from Reviewing to Testing Feb 20, 2023
@javierm javierm merged commit f7dfe30 into master Feb 20, 2023
Consul Democracy automation moved this from Testing to Release 2.0.0 Feb 20, 2023
@javierm javierm deleted the link_to_evaluate branch February 20, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Coadministration Management, Moderation and Valuation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants