As far as possible I think the code is clearer if we use CRUD actions
rather than custom actions. This will make it easier to add the action
to remove votes in the next commit.
Note that we are adding this line as we need to validate it that a vote
can be created on a comment by the current user:
```authorize! :create, Vote.new(voter: current_user, votable: @comment)```
We have done it this way and not with the following code as you might
expect, as this way two votes are created instead of one.
```load_and_authorize_resource through: :comment, through_association: :votes_for```
This line tries to load the resource @comment and through the association
"votes_for" it tries to create a new vote associated to that debate.
Therefore a vote is created when trying to authorise the resource and
then another one in the create action, when calling @comment.vote.
As far as possible I think the code is clearer if we use CRUD actions
rather than custom actions. This will make it easier to add the action
to remove votes in the next commit.
Note that we are adding this line as we need to validate it that a vote
can be created on a debate by the current user:
```authorize! :create, Vote.new(voter: current_user, votable: @debate)```
We have done it this way and not with the following code as you might
expect, as this way two votes are created instead of one.
```load_and_authorize_resource through: :debate, through_association: :votes_for```
This line tries to load the resource @debate and through the association
"votes_for" it tries to create a new vote associated to that debate.
Therefore a vote is created when trying to authorise the resource and
then another one in the create action, when calling @debate.vote_by (which
is called by @debate.register_vote).
This rule was added in rubocop-rspec 2.9.0.
We were using `be_nil` 50% of the time, and `be nil` the rest of the
time. No strong preference for either one, but IMHO we don't lose
anything be being consistent.
This rule was introduced in rubocop-rails 2.18.0.
Since using `response.parsed_body` is shorter than using
`JSON.parse(response.body)`, this also means we can group some lines in
one.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
Having expectations related to database operations in system tests after
the process running the browser has started might result in exceptions
while running our test suite.
For the HashAlignment rule, we're using the default `key` style (keys
are aligned and values aren't) instead of the `table` style (both keys
and values are aligned) because, even if we used both in the
application, we used the `key` style a lot more. Furthermore, the
`table` style looks strange in places where there are both very long and
very short keys and sometimes we weren't even consistent with the
`table` style, aligning some keys without aligning other keys.
Ideally we could align hashes to "either key or table", so developers
can decide whether keeping the symmetry of the code is worth it in a
case-per-case basis, but Rubocop doesn't allow this option.
It was possible to remove a map location from a different proposal (even
one created by a different author) by modifying the hidden `id`
parameter in the form.
So we're making sure the map location we destroy is the one associated
to the proposal we're updating.
Since we're now using the `@proposal` instance variable in the
`destroy_map_location_association` method, we're calling that method
after loading the resource with cancancan.
Some institutions using CONSUL have expressed interest in this feature
since some of their tenants might already have their own domains.
We've considered many options for the user interface to select whether
we're using a subdomain or a domain, like having two separate fields,
using a check box, ... In the end we've chosen radio buttons because
they make it easier to follow a logical sequence: first you decide
whether you're introducing a domain or subdomain, and then you enter it.
We've also considered hiding this option and assuming "if it's got a
dot, it's a domain". However, this wouldn't work with nested subdomains
and it wouldn't work with domains which are simply machine names.
Note that a group of radio buttons (or check boxes) is difficult to
style when the text of the label might expand over more than one line
(as is the case here on small screens); in this case, most solutions
result in the second line of the label appearing immediately under the
radio button, instead of being aligned with the first line of the label.
That's why I've added a container for the input+label combination.
Sometimes it might be convenient to use completely different views for
different tenants. For example, a certain tenant might use a footer that
has nothing to do with the default one.
For these cases, instead of adding `case Tenant.current_schema`
conditions to the view, it might be tidier to use a different file.
For this purpose, we're using Rails variants [1], which means that a
tenant named `mytenant` will use a template ending with
`.html+mytenant.erb` if it's available.
This works with components too, but has a limitation: when using the
`custom/` folder to add ERB files for a tenant, the default tenant ERB
file needs to be added to the `custom/` folder as well; if there aren't
changes to this file, a symbolic link will do.
For example, if we're writing a custom `admin/action_component` view for
the tenant `milky-way` but don't need to change this file for the
default tenant:
1. Create `app/components/custom/admin/action_component.rb` according to
the components customizations documentation [2]
2. Create the custom view for the `milky-way` tenant and save it under
`app/components/custom/admin/action_component.html+milky-way.erb`
3. Enter the `app/components/custom/admin/` folder and run `ln -s
../../admin/action_component.html.erb`
We're also adding some controller tests. Since Rails doesn't load the
middleware during controller tests, we're stubbing the `current_schema`
method directly instead of changing the subdomain of the request.
[1] https://guides.rubyonrails.org/v6.0/layouts_and_rendering.html#the-variants-option
[2] https://docs.consulproject.org/docs/english-documentation/customization/components
Note that the `create` action doesn't create an image but updates an
answer instead. We're removing the references to `:create` in the
abilities since it isn't used.
In the future we might change the form to add an image to an answer
because it's been broken for ages since it shows all the attached
images.
Adding, modifiying, and/or deleting questions for an already started
poll is far away from being democratic and can lead to unwanted side
effects like missing votes in the results or stats.
So, from now on, only modifiying questions will be possible only if
the poll has not started yet.
We were ignoring the draft version param when loading an annotation,
which could result in a strange situation where we load an annotation
and a draft version different than the one it belongs to.
Thanks to this change, we can simplify the code a little bit. IMHO the
`comments` and `new_comment` routes should have been added on member
instead of on collection, which would further simplify the code. I'm
leaving the routes untouched just in case changing the URL has side
effects on existing installations.
The idea to show the status of the existing features was done in commit
7339a98b74. Back then, we didn't have the separate `process.` prefix,
and so processes were enabled/disabled using settings like
`feature.debates` instead of `process.debates`.
IMHO making the information about the enabled features public could
potentially be a bit risky since it gives too much information about the
current status of the application.
Showing which processes are enabled, on the other hand, is pretty
harmless, and it's the reason why this feature was added in the first
place.
We add new method set_user_locale to render the page
with the user's preferred locale.
Note that we add a condition 'if params[:locale].blank?'
to recover the user's preferred locale. This is necessary
because it may be the case that the user does not have an
associated locale, and when execute '@user.locale' when
this value is 'nil', by default returns the default locale.
As we do not want this to happen and we want the locale we
receive as parameter to prevail in this case.
The user can access this page without being logged in.
We identify the user through the "subscriptions_token" parameter and
show a list of the notifications that can be enable/disable.
We will return a 404 error in case someone accesses the page with a
non-existent token.
We also control the case that some anonymous user tries to access the
page without any token, by returning the CanCan::AccessDenied exception.
The `hide` action was calling the `block` method while the `soft_block`
action was calling the `hide` method.
Combined with the fact that we also have a `block` permission which is
used in `ModerateActions` the logic was hard to follow.
Other than removing a redundant action, we're fixing two bugs when
blocking an author using the links in the public views:
* We were always redirecting to the debates index, even if we blocked
the author of a proposal or an investment
* We weren't showing any kind of success message
In commit 5a4921a1a we replaced `URI.parse` with `URI.open` due to some
issues during our tests with S3.
However, there are some security issues with `URI.open` [1], since it
might allow some users to execute code on the server.
So we're using `URI.parse#open` instead.
[1] https://docs.rubocop.org/rubocop/cops_security.html#securityopen
Both the calculate winners and delete actions benefit from some kind of
hint.
The "calculate winners" hint informs administrators that results won't
be publicly available unless the "show results" option is enabled.
The delete action was redirecting with an error message when the budget
couldn't be deleted; IMHO it's better to disable it and inform
administrators why it's disabled. Alternatively we could remove the
button completely; however, users might be looking for a way to delete a
budget and wouldn't find any hint about it.
We're now removing the "Delete" action from the budgets index table,
since most of the time it isn't possible to delete a budget and so the
action takes up space and we get little gain in return. We could keep
the "Delete" icon just for budgets which can be deleted; however, the
alignment of the table rows would suffer, making it harder to find the
intended action.
Before, users needed to navigate to the list of groups in order to
add, edit or delete a group.
Also, they need to navigate to the list of groups first, and then to
the list of headings for that group in order to add, edit or delete a
heading.
Now, it's possible to do all these actions for any group or heading
from the participatory budget view to bring simplicity and to reduce
the number of clicks from a user perspective.
Co-Authored-By: Javi Martín <javim@elretirao.net>
We're already using a custom controller to handle direct uploads.
Besides, as mentioned by one of Active Storage co-authors [1], the
Active Storage DirectUploadsController doesn't provide any
authentication or validation at all, meaning anyone could create blobs
in our database by posting to `/rails/active_storage/direct_uploads`.
The response there could be then used to upload any file (again, without
validation) to `/rails/active_storage/disk/`.
For now, we're monkey-patching the controllers in order to send
unauthorized responses, since we aren't using these routes. If we ever
enable direct uploads with Active Storage, we'll have to add some sort
of authentication.
Similar upload solutions like CKEditor don't have this issue since their
controllers inherit from `ApplicationController` (which includes
authorization rules), while Active Storage controllers inherit from
`ActionController::Base`.
[1] https://discuss.rubyonrails.org/t/activestorage-direct-uploads-safe-by-default-how-to-make-it-safe/74863/2
System tests are used to test the application from the user's point of
view. To test for specific exceptions, particularly regarding
authorization permissions, controller tests fit better.
Another option would be to test the page displayed shows a certain text,
like "Internal server error". I'm choosing controller tests because
they're faster and we're basically testing the same scenario many times
and we've already got a test checking what happens when users access a
page raising an exception.
So it's similar to the other custom page 404 tests.
Note we're explicitely marking the page as a draft so it's more obvious
what's going on in the test.
