Refactoring of Rails/HttpStatus #553
Merged
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
RuboCop will call the methods `#autocorrect_requested?`, `#support_autocorrect?` and `#autocorrect_enabled?` before performing autocorrect. It looks like `#support_autocorrect?` is the correct one to override.
08704e7 to
dbc1016
Compare
|
That's so much cleaner with your refactoring ! Wish I had figured out something like that by myself :) |
Match the send node in the cop as it was in the original, and extract the value used in the checker with One think that I don't like with those helper classes is the amount of duplication. On the other hand I find this approach very good for cops supporting different styles - we may refactor the rest and extract some common patterns in base checker classes. |
Good suggestions. I ended up pushing another 2 commits, which I haven’t squashed yet – should make it easier for you to review. |
87b6b21 to
81af38f
Compare
Extracts all style specific logic into two classes, `SymbolicStyleChecker` and `NumericStyleChecker`. This way, the chosen `style` is only checked once, instead of having every method contain the same `if ... else` statements.
81af38f to
5787a90
Compare
|
Thank you. Squashing and merging. |
|
@bquorning I would like to submit this same cop in the |
|
Yes absolutely, go ahead. The license even says something about “without limitation the rights to use, copy, modify, …” :-) |
8 tasks
bertocq
added a commit
to consuldemocracy/consuldemocracy
that referenced
this pull request
Mar 7, 2018
rubocop-rspec 1.23.0 release introduced the cop RSpec/Rails/HttpStatus to enforce consistent usage of the status format (numeric or symbolic). * rubocop/rubocop-rspec#553 * https://github.com/rubocop-rspec/rubocop-rspec/releases/tag/v1.23.0
bertocq
added a commit
to consuldemocracy/consuldemocracy
that referenced
this pull request
Apr 4, 2018
rubocop-rspec 1.23.0 release introduced the cop RSpec/Rails/HttpStatus to enforce consistent usage of the status format (numeric or symbolic). * rubocop/rubocop-rspec#553 * https://github.com/rubocop-rspec/rubocop-rspec/releases/tag/v1.23.0
bertocq
added a commit
to consuldemocracy/consuldemocracy
that referenced
this pull request
Apr 4, 2018
rubocop-rspec 1.23.0 release introduced the cop RSpec/Rails/HttpStatus to enforce consistent usage of the status format (numeric or symbolic). * rubocop/rubocop-rspec#553 * https://github.com/rubocop-rspec/rubocop-rspec/releases/tag/v1.23.0
bertocq
added a commit
to AyuntamientoMadrid/consul
that referenced
this pull request
Apr 4, 2018
rubocop-rspec 1.23.0 release introduced the cop RSpec/Rails/HttpStatus to enforce consistent usage of the status format (numeric or symbolic). * rubocop/rubocop-rspec#553 * https://github.com/rubocop-rspec/rubocop-rspec/releases/tag/v1.23.0
clairezed
added a commit
to CDJ11/CDJ
that referenced
this pull request
Jun 12, 2018
* Update CHANGELOG Unreleased section * Revert "Make config.time_zone configurable at secrets.yml" * Remove no longer usable investments rake task * Remove deprecated internal_comments column from Investments * Remove internal_comments usage in migration script Internal comments on migration script from SpendingProposal to Investments are no longer in use, so removal is best option. * Revert CHANGELOG release 0.14 Adding changes to a separate PR * Update CHANGELOG v0.14 * Update consul.json to v0.14 * Fix phone note english translation * Valuators access to edit/valute on right phase When a valuator tries to edit/valuate an investment outside valuating phase, an explanatory message will be shown along with a redirect to prevent access. * Fix investment creation for single budget usage Budget Investment factory creates a secondary budget as a collateral effect because it has a Heading factory that has a Group factory that creates a Budget. This was resulting in problems due to having two "active" Budgets created and `current_budget` method not choosing the one that we expected * updated test, changed fixed seeds for the new seed function * Fixes budgets ui for all phases * Adds link to username on admin users index view * Adds link to image and docs on admin budget investment info * Add Segment for users without supports on Budget Created not_supported_on_current_budget at UserSegment along with model spec scenario and translation strings. * Change the method have_css for find The old method have_css didn't wait, apparently, the Capybara's max_wait_time. It has been changed in favor of find, one that waits the stablished time for an element to appear in the screen. * Added test using have_content before selecting DNI or passport, the existence of the container is assured because have_conten waits for js to finish loading before checking * Update loofah gem to 2.2.1 version Details at flavorjones/loofah#144 * Find group inside budget We were having a problem were some groups where not updating correctly. That was because it was finding the first group with that name. However we were looking for another group with the same name from another budget Apart from the group_id, we also get the budget_id in the params for updating a group. By finding groups within that budget we get the expected behaviour * Order budget groups by id To maintain order after editing a group’s name We should probably add timestamps to `groups` and `headings` and order by `created_at` instead * Order budget headings by id To maintain order after editing a heading’s name We should probably add timestamps to `groups` and `headings` and order by `created_at` instead Could have done it in a separate PR, but gotta keep moving to other priority issues. Creating issue for timestamps to do correctly and with tests 😌 * Removes use of slugs to edit group name Changing a group’s `to_param` to return the slug instead of the id, breaks many tests in the user facing interface We should use slugs in upstream soon, but it should be done in a separate PR, bringing the whole slug implementation from Madrid’s fork and the corresponding test coverage * Remove obsolete flag_count No need for deprecation warnings in release notes, this is an attribute that was used temporarily but did not make it to a Release * Add max votable headings to groups * Allow voting in multiple headings Now that we have the option of voting in multiple headings per group, the method of voting in a “different heading assigned” has become deprecated and thus removed * Improve translation text and make it a default There was some inconsistency in this file, the `confirm_group` key was in both the custom translations file and the default translations file. Remove duplication and make it a default as it is a clearer message for users. If an installation want to edit this message they can still do it in the custom translations file * Consistent spacing Temporarily… because there are 2 kinds of Ruby developers, those who indent methods under `private` and does who don’t https://gist.github.com/joefiorini/1049083#gistcomment-37692 * Add headings_voted_by_user This method was used only in Madrid’s fork, but it is now needed to complete the backport for voting in multiple headings There wasn’t a test in Madrid, so here goes one too. Even though, the responsibility should probably be moved soon to the `Budget::Heading`. For consistency with the related methods and tests it has been left in the investment_spec * Fix edge case The user was able to vote as many investments as wanted in the first heading voted. However in the second heading voted, only one investment could be voted This was due to the previous implementation, where you could only vote in one heading. Note the `first` call in method `heading_voted_by_user?(user)` This commits simplifies the logic and allows voting for any investment in any heading that the user has previously voted in * Extend notifications to be marked as read and unread * Extend dev seeds to have notifications for all users Even though an action that triggers a notification is made, the notification is created in a separate step, reflecting how it is done in the corresponding controller https://github.com/AyuntamientoMadrid/consul/blob/master/app/controllers /comments_controller.rb#L16 * Adds styles for notifications views * Add Notifications partial to admin menu * Remove unrelated budget recommendation's link During the backport for “Read Notifications”[1] this link was added, which belongs to a different backport “Budget Recommendations” which is not quite ready to bring to upstream, yet 😌 [1] AyuntamientoMadrid#1304 * Fix dev seed specs * Add Node.js as requirement on README (spanish) * Fix read notifications translations * Add `map_location#json_data` method * Add `budget/investments#json_data` method * Add ajax request for marker investment info * Add `budget/investments#json_data` method permissions * Replace poltergeist with selenium-webdriver * Replace PhantomJS/Poltergeist config with Headless Chrome * Configure Travis CI to use Chrome addon, install ChromeDriver * Replace deprecated `.trigger('click')` API with `.click` * Use Selenium API to accept/dismiss JS modals/browser alerts JS modals/browser alerts are not automatically accepted now with Selenium, events that trigger such events must be wrapped in one of the following methods: `accept_alert`, `accept_confirm` or `dismiss_confirm` * Use absolute paths for fixtures * Disable JavaScript on IE-specific scenarios * Remove unnecessary status code related assertion * Replace deprecated `.trigger('mouseover')` API with `.hover` * Format dates with `.strftime('%d/%m/%Y')` when filling datepickers Advanced search scenarios for Budget::Investments, Debates and Proposals need proper date formatting as they behave unexpectedly when APIs such as `7.days.ago` are used * Fix failing spec on CI environments * Enable previously disabled test scenarios * Fix failing scenario related to Headless Chrome `window-size` flag * Fix failing scenario related to focused DOM element * Include ChromeDriver as prerequisite * Refactor test to avoid interaction with non-visible element * Remove unnecessary extra expectation for 'Voting in booth' scenario * Fix failing tests that simulated a click against the DOM The now-deprecated `.trigger('click')` API simulated a click against the DOM rather a click on the UI, which made our tests fragile and wouldn't simulate actual user interaction * Configure Capybara sessions to reset after each example * Refactor spec to use `let` syntax to DRY scenarios * Fix incorrect assertion for `nested imageable` example The example tests if a certain selector is hidden after adding one image but the assertion expected said selector to be visible, which made the scenario to fail at random * Refactor flaky tests to avoid interaction with the UI * Improve README code syntax * Fix notification expectations for read ones * Replace deprecated `to:` for `action:` at routes Running test suite the following appears: DEPRECATION WARNING: Defining a route where `to` is a symbol is deprecated. Please change `to: :json_data` to `action: :json_data`. * Isolate I18n failing tests * Update changelog * Isolate still failing tests * Add test suite for the feature The tests that will check the featute is working well added. There are four test: 1. Checks that the emails are been send to the user. The test looks for the link in there and takes the token. Visits the page and changes the password. 2 and 3. Both change the password by hand. One uses a password written by the manager, whilst the other uses the 'Generate random password' option. Both tests check that the user can sign in with the new passwords. 4. Checks that the password can be printed when it is saved. * Backend functionality to let managers update users password The back button when the user changes the password (in the print password page) redirects to the edit manually page. The routes to access password edit pages has been added, along with the ones to send reset password email and reset password manually. * Add UI to let manager change users password A submenu has been added to the side menu's 'Edit user account' option. This submenu has two options: - Reset password via email: an email is send so that the user can change their password by themselves. - Reset password manually: the manager has to write the password manually (or generate a random one). The passwords generated by the random password generator don't contain characters like $ or !. It uses some capital letters, some other lower case letters and some numbers. Ambiguous characters like 1, l, I has been removed. * Removes custom content on proposal index * Removes custom content on proposal show * Removes custom content on budget investment index * Removes custom i18n on budgets yml * Adds view mode i18n, styles and new icon * Adds missing i18n * Adds view mode on budget investments * Adds view mode on proposals * Adds view mode on debates * Fix english phone note translation * Add valuator groups * Assign valuators to groups * Assign groups to investments * Adds styles and missing i18n for valuator groups 👨🏻🎨 * Improves valuator groups index count * Adds missing i18n to valuator groups notices * Changes redirect path on create valuator group * Fixes specs * Display valuators on valuator group's show * Filter by valuator group * Remove description when creating valuator from index * Display valuator group assignments * Add valuation permissions to groups * Add group member count * Update rails-html-sanitizer gem version Security fix https://gemnasium.com/gems/rails-html-sanitizer/versions/1.0.4 * Clean up * Fix conflicts with fork * Fix specs * Fix change email address Not sure how this error creeped in 😕 probably a new gem version or other conflicting code The problem was we were getting an `unpermitted param email` when updating a user’s email address This stackoverflow solution seems to work nicely 😌 https://stackoverflow.com/questions/17384289/unpermitted-parameters-addi ng-new-fields-to-devise-in-rails-4-0#answer-19036427 * Fix Date & DateTime parsings to use default timezone Date.new(...) does not take into account the current timezone, while other parts of the application do. By default always parsing any date with the default timezone and converting the resulting Time to Date would prevent this kind of issues DateTime.parse(...).in_time_zone gives an unexpected result, as the DateTime.parse(...) will create a DateTime with +0000 time zone and the `in_time_zone` will modify the DateTime to adjust to the default zone. Maybe its better explained with an example, using 'Lima' as timezone: DateTime.parse("2015-01-01") > Thu, 01 Jan 2015 00:00:00 +0000 DateTime.parse("2015-01-01").in_time_zone > Wed, 31 Dec 2014 19:00:00 -05 -05:00 And that's not the desired date but the previous day! * Fix display of unfeasibility explanation We were missing a check to make sure valuation had finished before displaying the unfeasibility explanation * Use strings instead of method calls in expectations * Update Rubocop gem to 0.53.0 * Remove deprecated Performance/HashEachMethods cop At release https://github.com/bbatsov/rubocop/releases/tag/v0.53.0 it has been removed with rubocop/rubocop#5589 * Update rubocop-rspec gem to 1.24.0 from 1.22.1 * Enable new Rails/HttpStatus cop without issues rubocop-rspec 1.23.0 release introduced the cop RSpec/Rails/HttpStatus to enforce consistent usage of the status format (numeric or symbolic). * rubocop/rubocop-rspec#553 * https://github.com/rubocop-rspec/rubocop-rspec/releases/tag/v1.23.0 * Enable StaticAttributeDefinedDynamically cop & fix rubocop-rspec gem includes cops for FactoryBot like the new FactoryBot/StaticAttributeDefinedDynamically to enforce declaring static attribute values without a block. * http://www.rubydoc.info/gems/rubocop-rspec/1.24.0/RuboCop/Cop/RSpec/FactoryBot/StaticAttributeDefinedDynamically * Disable DynamicAttributeDefinedStatically cop rubocop-rspec includes a FactoryBot cop DynamicAttributeDefinedStatically that enforces declaring dynamic attribute values in a block. It was decided not to follow this convention. Explicitly disabling it gives more insight about current rubocop rules. http://www.rubydoc.info/gems/rubocop-rspec/1.24.0/RuboCop/Cop/RSpec/FactoryBot/DynamicAttributeDefinedStatically * Update Valencian translations * Update hebrew translations * Update french translations * Update catalan translation * Update nl translation * Add german translations * Update galician translations * Add indonesian translations * Update rubocop gem from 0.53.0 to 0.54.0 * Display message in budget's index when there are no budgets When there are no budgets we were seeing an exception in the budgets’ index There are two parts to take into account here: 1) Making sure there is a current_budget present, otherwise we display the “no budgets” message 2) The map helper is called from the controller, so we need to make sure current_budget is present there too Note: We could have added a bunch of `try` statements in the budgets’s index, instead of using a conditional, however there are quite a few `current_budget` calls so it seems more appropriate to use a conditional * Adds styles to no budgets message * Adds view mode on proposals index * Adds missing content to budget investments mode view This feature was already on Madrid fork and missing on backport * Validate ValuatorGroup#name presence & uniqueness Why: ValuatorGroup name should be unique and present to be able to identify correctly each of them. How: - Adding a presence & uniqueness validation at the model - Adding a sequenced value for name attribute at its factory - Adding missing model spec that covers validations * Fix specs * Adding Investment#by_valuator_group test scenario Budget::Investment#by_valuator_group scope didn't had a test scenario * Fix line length at admin investment controller * Fix Valuation Investment index heading filters Why: Heading filter where not being correctly displayed How: Increasing scenario to cover all possible combinations, and fixing the heading_filters method of the Valuation Budget Investment Controller to correctly: * Find how many investments the valuator can access * Count investments for each heading * Modified the investments partial to fit the new budget_investments UI: valuating filter name has changed to under_valuation. Modified the specs to fit the new UI for budget_investments * Fix conflics after rebase * Modifications to the spec to avoid using wait_for_ajax * Update PR with master Rebase master branch so that this PR can be updated with the latest changes. Conflicts has been solved and some specs updated to fit the new changes. dev_seeds has been also adapted to the new format. * Create setting to enable/disable attached documents Add setting to both seed and dev_seeds as well as a rake task to make it easier to set. * Translate admin setting banner sections * Disable document upload & show with new setting When Setting allow_attached_documents is disabled (false value) the user should not be able to upload documents neither see the documents lists * Move assigned_valuators from helper to model 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 * Add Valuator Groups list to investment csv & table We've added the list of valuator groups assigned to each investment at both admin investment list and investment csv exported file * Remove unnecesary parameter at Investment to_csv 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. * Extract Budget Investment to_csv to its own class 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. * Refactor Investment CSV export download scenario 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. * Refactor Investment csv download with filters test 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. * Remove duplicated mailer entry * Avoid db:dev_seed log print when run from its test The db:dev_seed rake logs info as it progresses as information for the developer. But that's not needed when ran from its tests file, and it bloats the travis/rspec output with unnecessary information. Now the task will always log info unless the rake task receives an optional argument. * Remove sitemap generator output when running specs When running tests there is no need to pollute rspec's output with any kind of log/info. * Fix MapLocation json_data to return mappable ids Until we correctly make MapLocation relation with mappables a polymorphic one... we'll need to return the investment_id and proposal_id values. Right now it was returning the MapLocation ID, and the JS was making a call searching for an Investment with the MapLocation ID... sometimes finding a record with same ID but totally NOT the one associated to the MapLocation. * Improves i18n of admin budgets * Adds class on back link to admin budget investments show * Moves h3 tag outside of table on polls results * Adds unicode_normalize for alt and title on images * Adds map skip checkbox i18n for budget investments * Adds message on polls index if there are no open polls * Fixes actions buttons on admin valuators index table * Shows message only if there is questions on legislation debate * Fixes missing i18n * add message views for unfeasible and not selected bugets investments * add locales (en) for unfeasible and not selected bugets investments * add test for unfeasible bugets investments * add test for not selected bugets investments * add missing dots (.) to config/locales * add locales (es) for unfeasible and not selected bugets investments * fix dentation * Fixes poll questions answer images absolute path spec * Add Globalize to Milestones * Add feature to delete a translation To delete a translation, a link has been added. This link works for the selected language. It hides all the things related to a language (the tab and the text_area) and empties the text area, so that the value is blank in the param hash. A variable called `delete_translations[]` is changed. e.g. If admin wants to remove English language, delete_translations[:en] will be 1; if not, it will be 0. When the milestone is updated, there is a before_action callback that cleans the selected languages for deletion (looking the delete_translations[] variable). Because of the deleted translations are blank in param hash, them won't be saved in DB. * Highlight current locale when changing locale from select When the locale changes the corresponding tab is highlighted automatically. When a language is added to the milestone, the tab is highlighted automatically. * Make portuguese locale work There was a problem with the portuguese locale. The locale was pt-BR, but `globalize_accessors` gem doesn't allow the creation of methods using locales with that format. To avoid transforming pt-BR to pt and lose the distinction of the different variations of the language, a function has been added to transform pt-BR into pt_br (without changing the locale itself). That way, when globalize uses the locales, all of them will have a valid format (downcased and underscored) AND they will be always the same (comparing pt-BR with pt_br doesn't work). * Use a helper with yield Globalize.with_locale A helper has been created to encapsule the logic of Globalize.with_locale. Now, to call that function, globalize(locale) do is called. * Modify specs to work with new features Add specs to check that the translations are being deleted correctly and the current locale tab is highlighted when the admin visits the edit milestone page. * Add dev_seeds for milestones with translations Add one milestone to each investment with translations for each locale defined in the app. * Refactorings - Cleanup Translatable module (`translation_params` method too large) - Move globalize_helpers partial to admin folder - Use any class for method translation_params - Helpers in `GlobalizeHelpers` make sure all are in use and see if they can be more legible - Review js name clases and methods see if they can be more legible - Refactor milestone views into partials with nice spacing between attributes * Improves admin content translatable ui * Remove gemnasium references, service just shut down Gemnasium has closed, service stopped. * Refs consuldemocracy#2603 Show 'See Results' button in admin panel * Add translations for the settings menu * Add logic to user verification changed functions on verification.rb, the first thing they do is return true whene skip_user_verification is active. changed show_welcome_screen? on user.rb, now its shows the welcome page even with te option active. changed welcome.html.erb, now if the user see this view and the option is activated, all 4 checks are green, not only 2. * Add default value for the setting on seeds/dev_seeds * Add test for all functinality new setting set to nil on test to prevent prevent unforeseen consequences on testing environment * Update README * Update CHANGELOG for release 0.15 * Add module name to CHANGELOG items * Update version number for consul.json * Fixes default or list view displaying * Fixes comments vote spec and management account spec * Update dev_seeds to cope with CJA specific users validation * Fix features management, votes and legislation specs * Fix features emails_spec * Fix voting multiple times in comments budget_investments and polls * Fix specs * Fix specs * Fix specs * Fix specs
clairezed
added a commit
to CDJ11/CDJ
that referenced
this pull request
Jun 26, 2018
* Update CHANGELOG Unreleased section * Revert "Make config.time_zone configurable at secrets.yml" * Remove no longer usable investments rake task * Remove deprecated internal_comments column from Investments * Remove internal_comments usage in migration script Internal comments on migration script from SpendingProposal to Investments are no longer in use, so removal is best option. * Revert CHANGELOG release 0.14 Adding changes to a separate PR * Update CHANGELOG v0.14 * Update consul.json to v0.14 * Fix phone note english translation * Valuators access to edit/valute on right phase When a valuator tries to edit/valuate an investment outside valuating phase, an explanatory message will be shown along with a redirect to prevent access. * Fix investment creation for single budget usage Budget Investment factory creates a secondary budget as a collateral effect because it has a Heading factory that has a Group factory that creates a Budget. This was resulting in problems due to having two "active" Budgets created and `current_budget` method not choosing the one that we expected * updated test, changed fixed seeds for the new seed function * Fixes budgets ui for all phases * Adds link to username on admin users index view * Adds link to image and docs on admin budget investment info * Add Segment for users without supports on Budget Created not_supported_on_current_budget at UserSegment along with model spec scenario and translation strings. * Change the method have_css for find The old method have_css didn't wait, apparently, the Capybara's max_wait_time. It has been changed in favor of find, one that waits the stablished time for an element to appear in the screen. * Added test using have_content before selecting DNI or passport, the existence of the container is assured because have_conten waits for js to finish loading before checking * Update loofah gem to 2.2.1 version Details at flavorjones/loofah#144 * Find group inside budget We were having a problem were some groups where not updating correctly. That was because it was finding the first group with that name. However we were looking for another group with the same name from another budget Apart from the group_id, we also get the budget_id in the params for updating a group. By finding groups within that budget we get the expected behaviour * Order budget groups by id To maintain order after editing a group’s name We should probably add timestamps to `groups` and `headings` and order by `created_at` instead * Order budget headings by id To maintain order after editing a heading’s name We should probably add timestamps to `groups` and `headings` and order by `created_at` instead Could have done it in a separate PR, but gotta keep moving to other priority issues. Creating issue for timestamps to do correctly and with tests 😌 * Removes use of slugs to edit group name Changing a group’s `to_param` to return the slug instead of the id, breaks many tests in the user facing interface We should use slugs in upstream soon, but it should be done in a separate PR, bringing the whole slug implementation from Madrid’s fork and the corresponding test coverage * Remove obsolete flag_count No need for deprecation warnings in release notes, this is an attribute that was used temporarily but did not make it to a Release * Add max votable headings to groups * Allow voting in multiple headings Now that we have the option of voting in multiple headings per group, the method of voting in a “different heading assigned” has become deprecated and thus removed * Improve translation text and make it a default There was some inconsistency in this file, the `confirm_group` key was in both the custom translations file and the default translations file. Remove duplication and make it a default as it is a clearer message for users. If an installation want to edit this message they can still do it in the custom translations file * Consistent spacing Temporarily… because there are 2 kinds of Ruby developers, those who indent methods under `private` and does who don’t https://gist.github.com/joefiorini/1049083#gistcomment-37692 * Add headings_voted_by_user This method was used only in Madrid’s fork, but it is now needed to complete the backport for voting in multiple headings There wasn’t a test in Madrid, so here goes one too. Even though, the responsibility should probably be moved soon to the `Budget::Heading`. For consistency with the related methods and tests it has been left in the investment_spec * Fix edge case The user was able to vote as many investments as wanted in the first heading voted. However in the second heading voted, only one investment could be voted This was due to the previous implementation, where you could only vote in one heading. Note the `first` call in method `heading_voted_by_user?(user)` This commits simplifies the logic and allows voting for any investment in any heading that the user has previously voted in * Extend notifications to be marked as read and unread * Extend dev seeds to have notifications for all users Even though an action that triggers a notification is made, the notification is created in a separate step, reflecting how it is done in the corresponding controller https://github.com/AyuntamientoMadrid/consul/blob/master/app/controllers /comments_controller.rb#L16 * Adds styles for notifications views * Add Notifications partial to admin menu * Remove unrelated budget recommendation's link During the backport for “Read Notifications”[1] this link was added, which belongs to a different backport “Budget Recommendations” which is not quite ready to bring to upstream, yet 😌 [1] AyuntamientoMadrid#1304 * Fix dev seed specs * Add Node.js as requirement on README (spanish) * Fix read notifications translations * Add `map_location#json_data` method * Add `budget/investments#json_data` method * Add ajax request for marker investment info * Add `budget/investments#json_data` method permissions * Replace poltergeist with selenium-webdriver * Replace PhantomJS/Poltergeist config with Headless Chrome * Configure Travis CI to use Chrome addon, install ChromeDriver * Replace deprecated `.trigger('click')` API with `.click` * Use Selenium API to accept/dismiss JS modals/browser alerts JS modals/browser alerts are not automatically accepted now with Selenium, events that trigger such events must be wrapped in one of the following methods: `accept_alert`, `accept_confirm` or `dismiss_confirm` * Use absolute paths for fixtures * Disable JavaScript on IE-specific scenarios * Remove unnecessary status code related assertion * Replace deprecated `.trigger('mouseover')` API with `.hover` * Format dates with `.strftime('%d/%m/%Y')` when filling datepickers Advanced search scenarios for Budget::Investments, Debates and Proposals need proper date formatting as they behave unexpectedly when APIs such as `7.days.ago` are used * Fix failing spec on CI environments * Enable previously disabled test scenarios * Fix failing scenario related to Headless Chrome `window-size` flag * Fix failing scenario related to focused DOM element * Include ChromeDriver as prerequisite * Refactor test to avoid interaction with non-visible element * Remove unnecessary extra expectation for 'Voting in booth' scenario * Fix failing tests that simulated a click against the DOM The now-deprecated `.trigger('click')` API simulated a click against the DOM rather a click on the UI, which made our tests fragile and wouldn't simulate actual user interaction * Configure Capybara sessions to reset after each example * Refactor spec to use `let` syntax to DRY scenarios * Fix incorrect assertion for `nested imageable` example The example tests if a certain selector is hidden after adding one image but the assertion expected said selector to be visible, which made the scenario to fail at random * Refactor flaky tests to avoid interaction with the UI * Improve README code syntax * Fix notification expectations for read ones * Replace deprecated `to:` for `action:` at routes Running test suite the following appears: DEPRECATION WARNING: Defining a route where `to` is a symbol is deprecated. Please change `to: :json_data` to `action: :json_data`. * Isolate I18n failing tests * Update changelog * Isolate still failing tests * Add test suite for the feature The tests that will check the featute is working well added. There are four test: 1. Checks that the emails are been send to the user. The test looks for the link in there and takes the token. Visits the page and changes the password. 2 and 3. Both change the password by hand. One uses a password written by the manager, whilst the other uses the 'Generate random password' option. Both tests check that the user can sign in with the new passwords. 4. Checks that the password can be printed when it is saved. * Backend functionality to let managers update users password The back button when the user changes the password (in the print password page) redirects to the edit manually page. The routes to access password edit pages has been added, along with the ones to send reset password email and reset password manually. * Add UI to let manager change users password A submenu has been added to the side menu's 'Edit user account' option. This submenu has two options: - Reset password via email: an email is send so that the user can change their password by themselves. - Reset password manually: the manager has to write the password manually (or generate a random one). The passwords generated by the random password generator don't contain characters like $ or !. It uses some capital letters, some other lower case letters and some numbers. Ambiguous characters like 1, l, I has been removed. * Removes custom content on proposal index * Removes custom content on proposal show * Removes custom content on budget investment index * Removes custom i18n on budgets yml * Adds view mode i18n, styles and new icon * Adds missing i18n * Adds view mode on budget investments * Adds view mode on proposals * Adds view mode on debates * Fix english phone note translation * Add valuator groups * Assign valuators to groups * Assign groups to investments * Adds styles and missing i18n for valuator groups 👨🏻🎨 * Improves valuator groups index count * Adds missing i18n to valuator groups notices * Changes redirect path on create valuator group * Fixes specs * Display valuators on valuator group's show * Filter by valuator group * Remove description when creating valuator from index * Display valuator group assignments * Add valuation permissions to groups * Add group member count * Update rails-html-sanitizer gem version Security fix https://gemnasium.com/gems/rails-html-sanitizer/versions/1.0.4 * Clean up * Fix conflicts with fork * Fix specs * Fix change email address Not sure how this error creeped in 😕 probably a new gem version or other conflicting code The problem was we were getting an `unpermitted param email` when updating a user’s email address This stackoverflow solution seems to work nicely 😌 https://stackoverflow.com/questions/17384289/unpermitted-parameters-addi ng-new-fields-to-devise-in-rails-4-0#answer-19036427 * Fix Date & DateTime parsings to use default timezone Date.new(...) does not take into account the current timezone, while other parts of the application do. By default always parsing any date with the default timezone and converting the resulting Time to Date would prevent this kind of issues DateTime.parse(...).in_time_zone gives an unexpected result, as the DateTime.parse(...) will create a DateTime with +0000 time zone and the `in_time_zone` will modify the DateTime to adjust to the default zone. Maybe its better explained with an example, using 'Lima' as timezone: DateTime.parse("2015-01-01") > Thu, 01 Jan 2015 00:00:00 +0000 DateTime.parse("2015-01-01").in_time_zone > Wed, 31 Dec 2014 19:00:00 -05 -05:00 And that's not the desired date but the previous day! * Fix display of unfeasibility explanation We were missing a check to make sure valuation had finished before displaying the unfeasibility explanation * Use strings instead of method calls in expectations * Update Rubocop gem to 0.53.0 * Remove deprecated Performance/HashEachMethods cop At release https://github.com/bbatsov/rubocop/releases/tag/v0.53.0 it has been removed with rubocop/rubocop#5589 * Update rubocop-rspec gem to 1.24.0 from 1.22.1 * Enable new Rails/HttpStatus cop without issues rubocop-rspec 1.23.0 release introduced the cop RSpec/Rails/HttpStatus to enforce consistent usage of the status format (numeric or symbolic). * rubocop/rubocop-rspec#553 * https://github.com/rubocop-rspec/rubocop-rspec/releases/tag/v1.23.0 * Enable StaticAttributeDefinedDynamically cop & fix rubocop-rspec gem includes cops for FactoryBot like the new FactoryBot/StaticAttributeDefinedDynamically to enforce declaring static attribute values without a block. * http://www.rubydoc.info/gems/rubocop-rspec/1.24.0/RuboCop/Cop/RSpec/FactoryBot/StaticAttributeDefinedDynamically * Disable DynamicAttributeDefinedStatically cop rubocop-rspec includes a FactoryBot cop DynamicAttributeDefinedStatically that enforces declaring dynamic attribute values in a block. It was decided not to follow this convention. Explicitly disabling it gives more insight about current rubocop rules. http://www.rubydoc.info/gems/rubocop-rspec/1.24.0/RuboCop/Cop/RSpec/FactoryBot/DynamicAttributeDefinedStatically * Update Valencian translations * Update hebrew translations * Update french translations * Update catalan translation * Update nl translation * Add german translations * Update galician translations * Add indonesian translations * Update rubocop gem from 0.53.0 to 0.54.0 * Display message in budget's index when there are no budgets When there are no budgets we were seeing an exception in the budgets’ index There are two parts to take into account here: 1) Making sure there is a current_budget present, otherwise we display the “no budgets” message 2) The map helper is called from the controller, so we need to make sure current_budget is present there too Note: We could have added a bunch of `try` statements in the budgets’s index, instead of using a conditional, however there are quite a few `current_budget` calls so it seems more appropriate to use a conditional * Adds styles to no budgets message * Adds view mode on proposals index * Adds missing content to budget investments mode view This feature was already on Madrid fork and missing on backport * Validate ValuatorGroup#name presence & uniqueness Why: ValuatorGroup name should be unique and present to be able to identify correctly each of them. How: - Adding a presence & uniqueness validation at the model - Adding a sequenced value for name attribute at its factory - Adding missing model spec that covers validations * Fix specs * Adding Investment#by_valuator_group test scenario Budget::Investment#by_valuator_group scope didn't had a test scenario * Fix line length at admin investment controller * Fix Valuation Investment index heading filters Why: Heading filter where not being correctly displayed How: Increasing scenario to cover all possible combinations, and fixing the heading_filters method of the Valuation Budget Investment Controller to correctly: * Find how many investments the valuator can access * Count investments for each heading * Modified the investments partial to fit the new budget_investments UI: valuating filter name has changed to under_valuation. Modified the specs to fit the new UI for budget_investments * Fix conflics after rebase * Modifications to the spec to avoid using wait_for_ajax * Update PR with master Rebase master branch so that this PR can be updated with the latest changes. Conflicts has been solved and some specs updated to fit the new changes. dev_seeds has been also adapted to the new format. * Create setting to enable/disable attached documents Add setting to both seed and dev_seeds as well as a rake task to make it easier to set. * Translate admin setting banner sections * Disable document upload & show with new setting When Setting allow_attached_documents is disabled (false value) the user should not be able to upload documents neither see the documents lists * Move assigned_valuators from helper to model 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 * Add Valuator Groups list to investment csv & table We've added the list of valuator groups assigned to each investment at both admin investment list and investment csv exported file * Remove unnecesary parameter at Investment to_csv 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. * Extract Budget Investment to_csv to its own class 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. * Refactor Investment CSV export download scenario 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. * Refactor Investment csv download with filters test 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. * Remove duplicated mailer entry * Avoid db:dev_seed log print when run from its test The db:dev_seed rake logs info as it progresses as information for the developer. But that's not needed when ran from its tests file, and it bloats the travis/rspec output with unnecessary information. Now the task will always log info unless the rake task receives an optional argument. * Remove sitemap generator output when running specs When running tests there is no need to pollute rspec's output with any kind of log/info. * Fix MapLocation json_data to return mappable ids Until we correctly make MapLocation relation with mappables a polymorphic one... we'll need to return the investment_id and proposal_id values. Right now it was returning the MapLocation ID, and the JS was making a call searching for an Investment with the MapLocation ID... sometimes finding a record with same ID but totally NOT the one associated to the MapLocation. * Improves i18n of admin budgets * Adds class on back link to admin budget investments show * Moves h3 tag outside of table on polls results * Adds unicode_normalize for alt and title on images * Adds map skip checkbox i18n for budget investments * Adds message on polls index if there are no open polls * Fixes actions buttons on admin valuators index table * Shows message only if there is questions on legislation debate * Fixes missing i18n * add message views for unfeasible and not selected bugets investments * add locales (en) for unfeasible and not selected bugets investments * add test for unfeasible bugets investments * add test for not selected bugets investments * add missing dots (.) to config/locales * add locales (es) for unfeasible and not selected bugets investments * fix dentation * Fixes poll questions answer images absolute path spec * Add Globalize to Milestones * Add feature to delete a translation To delete a translation, a link has been added. This link works for the selected language. It hides all the things related to a language (the tab and the text_area) and empties the text area, so that the value is blank in the param hash. A variable called `delete_translations[]` is changed. e.g. If admin wants to remove English language, delete_translations[:en] will be 1; if not, it will be 0. When the milestone is updated, there is a before_action callback that cleans the selected languages for deletion (looking the delete_translations[] variable). Because of the deleted translations are blank in param hash, them won't be saved in DB. * Highlight current locale when changing locale from select When the locale changes the corresponding tab is highlighted automatically. When a language is added to the milestone, the tab is highlighted automatically. * Make portuguese locale work There was a problem with the portuguese locale. The locale was pt-BR, but `globalize_accessors` gem doesn't allow the creation of methods using locales with that format. To avoid transforming pt-BR to pt and lose the distinction of the different variations of the language, a function has been added to transform pt-BR into pt_br (without changing the locale itself). That way, when globalize uses the locales, all of them will have a valid format (downcased and underscored) AND they will be always the same (comparing pt-BR with pt_br doesn't work). * Use a helper with yield Globalize.with_locale A helper has been created to encapsule the logic of Globalize.with_locale. Now, to call that function, globalize(locale) do is called. * Modify specs to work with new features Add specs to check that the translations are being deleted correctly and the current locale tab is highlighted when the admin visits the edit milestone page. * Add dev_seeds for milestones with translations Add one milestone to each investment with translations for each locale defined in the app. * Refactorings - Cleanup Translatable module (`translation_params` method too large) - Move globalize_helpers partial to admin folder - Use any class for method translation_params - Helpers in `GlobalizeHelpers` make sure all are in use and see if they can be more legible - Review js name clases and methods see if they can be more legible - Refactor milestone views into partials with nice spacing between attributes * Improves admin content translatable ui * Remove gemnasium references, service just shut down Gemnasium has closed, service stopped. * Refs consuldemocracy#2603 Show 'See Results' button in admin panel * Add translations for the settings menu * Add logic to user verification changed functions on verification.rb, the first thing they do is return true whene skip_user_verification is active. changed show_welcome_screen? on user.rb, now its shows the welcome page even with te option active. changed welcome.html.erb, now if the user see this view and the option is activated, all 4 checks are green, not only 2. * Add default value for the setting on seeds/dev_seeds * Add test for all functinality new setting set to nil on test to prevent prevent unforeseen consequences on testing environment * Update README * Update CHANGELOG for release 0.15 * Add module name to CHANGELOG items * Update version number for consul.json * Fixes default or list view displaying * Fixes comments vote spec and management account spec * Update dev_seeds to cope with CJA specific users validation * Fix features management, votes and legislation specs * Fix features emails_spec * Fix voting multiple times in comments budget_investments and polls * Fix specs * Fix specs * Fix specs * Fix specs
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.
As mentioned in #546, I was never completely happy with the
Rails/HttpStatuscop, so I finally got around to refactoring it.By extracting all style specific logic into two classes (
SymbolicStyleCheckerandNumericStyleChecker), the configuredstyleis only checked once, instead of every method having identicalif style == :foo ... else ...statements.I hope you can help me come up with better names for these “checker” classes. And if you have suggestions on how to avoid having
def_node_matchercalled on the singleton class, please let me know.By the way don’t try to read the diff. Better to look separately at the cop before and after.
cc @anthony-robin
Before submitting the PR make sure the following are checked:
master(if not - rebase it).bundle exec rake) is passing.