We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
In general, we always use relative URLs (using `_path`), but sometimes
we were accidentally using absolute URLs (using `_url`). It's been
reported i might cause some isuses if accepting both HTTP and HTTPS
connections, although we've never seen the case.
In any case, this change makes the code more consistent and makes the
generated HTML cleaner.
It's possible to have a given order greater than the number of answers;
we don't have any validation rules for that. So the check for the number
of answers isn't enough.
Checking the maximum given order in the answers is safer. Another option
would be to reorder the answers every time we add a new one, but I'm not
sure whether that's the expected behaviour.
Note even after this change the action is not thread-safe, as it is
possible to create two questions with the same given order with two
simultaneous requests.
Using the `_html` suffix automatically marks texts as HTML safe, so
doing so on sanitized texts is redundant.
Note flash texts are not sanitized the moment they are generated, but
are sanitized when displayed in the view.
We use this method in two different scenarios. In an AJAX request, we
don't want to return every booth if the search is blank. However, in a
normal HTTP GET request, we want to return every record when the search
is empty, as we do everywhere else.
It's possible the behaviour of the AJAX call is unusual, since it
searches all booths, and not just the ones assigned to a poll. If we
changed this behaviour, we could simplify the code and remove the
`quick_search` method.
We were monkey-patching FoundationRailsHelper::Formbuilder, which made
form customization difficult. We can inherit from it, which is the
standard way of extending what an existing class does, and make our form
the default one.
Naming two variables the same way is confusing at the very least, and
can lead to hard to debug errors. That's why the Ruby interpreter issues
a warning when we do so.
Moderate legislation proposals
- added a controller for moderation/legislation
- updated view to appropriate link + added route
- added a spec
- Feature test
- test for faded
- javascripts for visual effects
The code `where(id: ids)` is equivalent to `where(id: ids.uniq)`.
Since Rails 5 uses `distinct` instead of `uniq` and in most cases where
we use `uniq` with `pluck` we should simply remove the `uniq` call (as
done in this commit), we're also removing the `Rails/UniqBeforePluck`
rubocop rule.
The statement executed the method twice if the `present?` condition was
true. If the condition was false, it executed it once anyway.
It's probably a typo and originally we probably meant we wanted to
execute the method if the method existed.
- Add :date_of_birth and :postal_code
- Only display new fields when aplication has configured the Remote
Census API and contains values for fields. Check with Setting Class
methods:
- force_presence_date_of_birth?
- force_presence_postal_code?
This new functionality will allow to retrieve in the signature sheet
the document number, the date of birth and the postal code.
So we renamed :document_numbers to :required_fields_to_veriry to
clarify and adjust the name to its use.
- Add :date_of_birth and :postal_code
- Only display new fields when aplication has configured the
custom census API and contains alias values for fields. Add 2
class Setting methods to check this feature:
- force_presence_date_of_birth?
- force_presence_postal_code?
Currently after each update of any Settings is redirected to the first
tab by default.
As this new tab remote_census_configuation has a lot of fields to fill
in it is a bit uncomfortable to have to go back to the tab after each
update.
- Add hidden field :tag to set current tag value
- After update add tag value to request.referer
- To avoid errors when partial call has not param :tag, add the "define?"
method on hidden_field value.
- Render remote census configuration content on settings index.
- Update type method from Setting
On Admin::SettingsController#index we are using 'all_settings' to
group all settings by 'type' method.
'type' method return the first part of key when split by '.'
To allow use by example: all_settings["remote_census.general"]
and recover only settings related with this key we have added new
'elsif' on 'type' method.
After adding new namespace `admin/local_census_records` cancancan
`load_and_authorize` method started to crash because it was getting
confused when loading and authorizing controller resources. Specifying
class to load and authorize solves this situation.
