Add valuator groups assigned to investments to admin tables & csv export #2592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's no good reason to call assigned_valuators(investment) when the logic can be at the model. Also removed the valuator_groups texts to be added in an independent method
We've added the list of valuator groups assigned to each investment at both admin investment list and investment csv exported file
If there's only one usage of `to_csv` and the parameter has always the same value... there's no good reason to bother using an additional argument.
The csv generation doesn't seem like a Model concern, at least not taking into account the amount of lines of the method (36+). Just a simple ruby class that encapsulates the logic makes it easier to read and maintain as we increase the columns exported.. also customize in case other forks need different values.
The created investment didn't had all data to correctly assert each column values are correctly exported. The expectations checking if each particular text appears are invalid in this test. The objective is to check that the downloaded file contents are exactly as they should be... not particular parts checked in an independent way as for example "Yes" could appear in two different columns and just looking if the text appears is not a valid assertion.
There's no need to check again headers in this scenario, previous one already does it. Correctly naming variables, as well as using explicit expectations is a good idea. Last but not least, expectations where reversed but by luck or lack of attention where passing.
MariaCheca
approved these changes
Apr 11, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
This is a backport & refactor from madrid's fork AyuntamientoMadrid#1287 & AyuntamientoMadrid#1422
Objectives
Valuator Groups are now assigned to Investments, and need to be displayed in both:
Visual Changes (if any)
Yes, check referenced PR's
Notes
Maybe too much refactor on a single PR... but we're trying to cleanup pending backports to be able to merge latest changes around this CSV export.