In theory it's possible to add a `host` parameter to a URL, and we could
end up redirecting to that host if we just redirect using query
parameters.
Generating the path using `url_for` with `only_path` solves the issue.
Note in the tests I'm using the `get` method because the `patch` method
wouldn't send query parameters. This doesn't mean the action can be
accessed through GET requests, since controller tests don't check route
verbs. Using feature specs doesn't seem to work because `controller` and
`host` parameters are filtered automatically in feature specs.
Also note I'm not testing every hidden/moderation controller because
they basically use the same code.
These actions are never called with query parameters in our application,
so there's no need to use these parameters in a redirect.
Note in the test I'm using the `get` method because the `patch` method
wouldn't send query parameters. This doesn't mean the action can be
accessed through GET requests, since controller tests don't check route
verbs.
Creating more than 25 records isn't necessary to test pagination; we can
stub the number of records per page in a test.
On my machine we save about one second per test with these changes.
The scopes `created_by_admin` and `public_polls` were very similar. I'm
using `created_by_admin` because `Poll.public_polls` feels redundant,
and the reason for that name is we should not name the scope `public`
because `public` is a ruby access modifier.
This feature wasn't properly tested nor reviewed, and after reviewing
several pull requests with a similar status and considering this pull
request is related to the public area of the web, we've decided to
remove it before releasing version 1.1.
This commit reverts commit 4f50e67a.
Although we weren't showing links in the views to execute certain
actions, forms could be still sent using a PUT/PATCH pull request to the
controller actions.
The new CSV report was more configurable and could work on proposals,
processes and comments. However, it had several issues.
In the public area, by default it generated a blank file.
In the admin section, the report was hard to configure and it generated
a file with less quality than the old system.
So until we improve this system, we're bringing back the old investment
CSV exporter.
This commit reverts most of commit 9d1ca3bf.
Our manual implementation had a few issues. In particular, it didn't
track changes related to associations, which became more of an issue
when we made investments translatable.
Using audited gives us more functionality while at the same time
simplifies our code. However, it adds one more external dependency to
our project.
The reason for choosing audited over paper trail is audited seems to
make it easier to handle associations.
The current tracking section had a few issues:
* When browsing as an admin, this section becomes useless since no
investments are shown
* Browsing investments in the admin section, you're suddenly redirected
to the tracking section, making navigation confusing
* One test related to the officing dashboard failed due to these changes
and had been commented
* Several views and controller methods were copied from other sections,
leading to duplication and making the code harder to maintain
* Tracking routes were defined for proposals and legislation processes,
but in the tracking section only investments were shown
* Probably many more things, since these issues were detected after only
an hour reviewing and testing the code
So we're removing this untested section before releasing version 1.1. We
might add it back afterwards.
Tags and help links can be edited, but aren't used anywhere. Since we
don't know what the intended behavior was, I'm removing them for now.
My best guess is tags were supposed to be used so investments for a
budget can only be assigned tags present in the budget. Achieving that
behavior wouldn't be a trivial task.
We were manually doing the same thing, generating inconsistent results,
since the method `valuation_tag_list` was using the `valuation` context,
when actually the expected behavior would be to use the `valuation_tag`
context.
We were using two different systems to set translations in JavaScript:
to set the text for languages, we were using data attributes, and to set
the text for staff members, we were using AJAX calls.
I find data attributes keep the code more simple, since there's no need
to define an extra route and controller action. Furthermore, the user
experience is better because response times are faster.
So now both places use data attributes.
Unfortunately this feature wasn't properly reviewed and tested, and it
had many bugs, some of them critical and hard to fix, like validations
being skipped in concurrent requests.
So we're removing it before releasing version 1.1. We might add it back
in the future if we manage to solve the critical issues.
This commit reverts commit 836f9ba7.
This method is ambiguous. Sometimes we use it to set invalid data in
tests (which can usually be done with `update_column`), and other times
we use it instead of `update!`.
I'm removing it because, even if sometimes it could make sense to use
it, it's too similar to `update_attributes` (which is an alias for
`update` and runs validations), making it confusing.
However, there's one case where we're still using it: in the
ActsAsParanoidAliases module, we need to invoke the callbacks, which
`update_column` skips, but tests related to translations fail if we use
`update!`. The reason for this is the tests check what happens if we
restore a record without restoring its translations. But that will make
the record invalid, since there's a validation rule checking it has at
least one translation.
I'm not blacklisting any other method which skips validations because we
know they skip validations and use them anyway (hopefully with care).
We were inconsistent on this one. I consider it particularly useful when
a method starts with a `return` statement.
In other cases, we probably shouldn't have a guard rule in the middle of
a method in any case, but that's a different refactoring.
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.
