The action and the views were almost identical, with the supports
progress and the HTML classes of the success message element being the
only exceptions; we can use CSS for the styles instead.
Just like we did in commit 0214184b2d for investments, we're removing
some possible optimizations (we don't have any benchmarks proving they
affect performance at all) in order to simplify the code.
The investement votes component `delegate` code was accidentally left
but isn't used since commit 0214184b2, so we're removing it now that
we're removing the `voted_for?` helper method.
The `legislation_proposals#index` action was never used because it used
the same URL as `legislation_processes#proposals`.
In commit 702bfec24 we removed the view, but we forgot to remove the
controller action, the route, and some partials which were rendered from
the index view.
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.
You can update the same "notifications" section that we allow you to
update in "my account".
This "subscriptions" section differs from the "my account" section
because we do not need to be logged in to update the status of the
notifications.
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 test "Action links remember the pagination setting and the filter"
was failing sometimes because it assumed the third user created was
going to appear in the third place, but that wasn't always the case.
So we're using the same order we use in the rest of the sections dealing
with hidden content.
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
We're going to add geozones as user segments, so it's handy to have the
method in the UserSegments class.
We're also changing the `user_segment_emails` parameter name for
consistency and simplicity.
The link to "See all investments" didn't have the `heading_id`
parameter, which resulted in the ballot information not being displayed
when in the voting phase.
We could modify the link to include the `heading_id` parameter, but IMHO
it's more robust to select the heading automatically when there's only
one heading. That way manually accessing the page without a `heading_id`
parameter will still work as if the heading had been selected.
The interface was a bit confusing, since after clicking on "See
unfeasible investments" (or similar), we were on a page where no
investments were shown.
Besides, since commit 7e3dd47d5, the group page is only linked from the
"my ballot" page, through a link inviting the user to vote in that
group, and it's only possible to vote selected investments (which is the
default filter during the final voting phase).
The only reason we had these links here was these links weren't present
in the investments page. But they're present there since commit
04605d5d5, so we don't need them in the group page anymore.
As mentioned in commit bc0f04075, a <select> field which submits its
form on change causes many accessibility and usability issues. In this
case there was also an incompatibility with the advanced search filter
which caused a bug solved in commit 541a5fa89.
So the question is where to position the filters and how to display
them. One factor to take into the account is how relevant these filters
are, particularly compared to the links to select the prefered order,
since we don't usually give users the choice of both filters and orders.
Our filters don't really make sense until the valuation phase starts,
since before that phase investments aren't selected nor their
feasibility is decided.
After that phase, the only phase where citizens are really involved
is the final voting; the rest of the phases are done by valuators and
administrators. In the final voting, citizens can only vote on selected
projects, and that's the default filter during that phase.
So these filters are mainly there for information purposes, and not to
help citizens in the phases where they're actually involved (accepting
projects, selecting projects and balloting).
Orders, on the other hand, play a crucial role during the final voting
phase. Since citizens might have already voted for a few projects and
have, let's say, 100,000€ left, ordering by price allows them to find
which projects are within their remaining budget.
In conclusion, orders are more important than filters, and so they
should have a more prominent place.
For consistency with the proposals section, where we've got some links
in the sidebar (bottom part of the page on small screens) providing a
similar funcionality, like accessing selected proposals or archived or
retired proposals, we're moving the investments filters to the sidebar
(bottom part of the page on small screens) as well.
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.
Calculating winners before the balloting is over is useless (results
aren't published at that point) and can lead to the wrong results since
users are still voting and results might change.
And we were showing the button to calculate winners even when a budget
had finished. However, in this case the action to calculate winners did
nothing, which resulted in administrators seeing nothing happened after
pressing the button.
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>
In the past it would have been confusing to add a way to directly
enable/disable a phase in the phases table because it was in the middle
of the form. So we would have had next to each other controls that don't
do anything until the form is sent and controls which modify the
database immediately. That's why we couldn't add the checkboxes we used
when using the wizard.
Now the phases aren't on the same page as the budget form, so we can
edit them independently. We're using a switch, so it's consistent with
the way we enable/disable features. We could have used checkboxes, but
with checkboxes, users expect they aren't changing anything until they
click on a button to send the form, so we'd have to add a button, and it
might be missed since we're going to add "buttons" for headings and
groups to this page which won't send a form but will be links.
Since we're changing the element with JavaScript after an AJAX call, we
need a way to find the button we're changing. The easiest way is adding
an ID attribute to all admin actions buttons/links.
Having links in the middle of a form distracts users from the task of
filling in the form, and following a link before submitting the form
will mean whatever has been filled in is lost.
And the budgets form is already very long and hard to fill in. Having
the phases table in the middle of it made it even harder. And, since
we're planning to add the option to manage groups and headings from the
same page, it's better to have a dedicated page for the form.
We weren't showing the details of answers without a description, even if
they had images, videos or documents. Some users found that behavior
unexpected since the description isn't a mandatory field and so they
left it blank, but they added images to that answer and they didn't
appear on the poll page.
Note we had a condition not to show the title of an answer when it had
no description. I think that condition was redundant because answers
without a description weren't loaded in the first place. Anyway, that
condition doesn't make sense anymore because we're displaying answers
with images but no description.
It could be argued that seeing which proposals a user follows is a good
indicator of which proposals a user has supported, since we're
automatically creating follows for supported proposals since commit
74fbde09f. So now, we're extending the `public_interests` funcionality,
so it only shows elements users are following if they've enabled it.
This is an improvement over using the `public_activity` attribute in two
ways:
* The `public_interests` attribute is disabled by default, so by default
other users won't be able to see what a user is following
* Who has created proposals/debates/investments/comments is public
information, while who is following which elements is not; so enabling
`public_activity` shouldn't imply potentially private information should
be displayed as well
We've considered removing the `public_interests` attribute completely
and just hiding the "following" page for everyone except its owner, but
keeping it provides more compatibility with existing installations.
We had similar conditions many times and some duplication, particularly
between the code getting records and the code counting records. We can
remove it by returning a generic scope and calling the `count` method to
count the records and the `order` and `page` methods when we want to
pass the records to the view. And, since we only display one partial per
request, we can simplify the code to render all the partials. There's
one minor disadvantage to this approach: searching the code for the
place where these partials are rendered is now a bit harder since
searching the code for something like "render (.+) budget_investments"
won't return any results.
We're also writing conditions about a certain filter just once, by
setting `valid_filters`. This greatly simplifies the logic, although
again there's one minor disadvantage: we have to implement the
`current_filter` method, duplicating the logic already defined in the
`HasFilters` concern.
In order to migrate existing files from Paperclip to ActiveStorage, we
need Paperclip to find out the files associated to existing database
records. So we can't simply replace Paperclip with ActiveStorage.
That's why it's usually recommended [1] to first run the migration and
then replace Paperclip with ActiveStorage using two consecutive
deployments.
However, in our case we can't rely on two consecutive deployments
because we have to make an easy process so existing CONSUL installations
don't run into any issues. We can't just release version 1.4.0 and 1.5.0
and day and ask everyone to upgrade twice on the same day.
Instead, we're following a different plan:
* We're going to provide a Rake task (which will require Paperclip) to
migrate existing files
* We still use Paperclip to generate link and image tags
* New files are handled using both Paperclip and ActiveStorage; that
way, when we make the switch, we won't have to migrate them, and in
the meantime they'll be accessible thanks to Paperclip
* After we make the switch, we'll update the `name` column in the active
storage attachments tables in order to remove the `storage_` prefix
Regarding our handling of new files, the exception are cached
attachments. Since those attachments are temporary files used while
submitting a form and we have to delete them afterwards, we're only
handling them with Paperclip. We'll handle these ones in version 1.5.0.
Note the task creating the dev seeds was failing after these changes
with an `ActiveStorage::IntegrityError` exception because we were
opening some files without closing them. If the same file was attached
twice, it failed the second time.
We're solving it by closing the files with `File.open` and a block. Even
though we didn't get any errors, we're doing the same thing in the
`Attachable` concern because it's a good practice to close files after
we're done with them.
Also note we have to change the CKEditor Active Storage code so it's
compatible with Paperclip. In this case, I haven't been able to write a
test to confirm the attachment exists; I was getting the same
`ActiveStorage::IntegrityError` mentioned above.
Finally, we're updating the site customization image controller to use
`update` so the image and the attachment are updated within the same
transaction. This is also what we do in most controllers.
[1] https://www.youtube.com/watch?v=tZ_WNUytO9o
Having to wait for a whole page refresh after updating each setting was
painful when modifying several settings.
Even though the navigation is updated immediately to reflect which
sections have been enabled/disabled, there's one gotcha. Changing the
"SDG" setting will not update the user menu (which contains a link to
SDG content) nor the "SDG Configuration" tab; refreshing the page will
be necessary to check these changes. The same happens with the map and
remote census tabs. So in these cases we're making an exception and
sending the form. We might find a better solution in the future.
For this reason, we aren't using the `switch` ARIA role. Some users
might not expect a switch control to refresh the page, just like they
usually don't expect checkboxes to refresh the page. Furthermore, screen
reader support for the `switch` role seems to be inconsistent. For
instance, NVDA with Chrome announces the control as a checkbox instead
of a switch.
Note AJAX is only used for feature settings. Other settings are still
updated with regular HTTP requests.
Since we're now using AJAX requests, we have to make sure to add an
expectation in the homepage tests in order to make sure the request has
finished before starting a new one.
We're not adding the rule because it would apply the current line length
rule of 110 characters per line. We still haven't decided whether we'll
keep that rule or make lines shorter so they're easier to read,
particularly when vertically splitting the editor window.
So, for now, I'm applying the rule to lines which are about 90
characters long.
We were already using it in most places. Since rubocop-rails 2.11.0,
this rule also detects offenses when using the `head` method, which we
were using with a plain `404`.
This way the code is easier to follow; the code checking whether the
list has contents is in the partial rendering the list.
We also remove some duplication setting up related content in the
controllers.
For some reason, we have to manually ignore i18n keys which were
automatically ignored when the code was in the view.
Since PostgreSQL doesn't guarantee the order of the records unless an
`ORDER BY` clause is used, the records sometimes had a different order.
This might cause records being present in more than one page.
One test was failing sometimes (when the record created first was
rendered after the record created last) because there seems to be a bug
in chrome/chromedriver 83 and later which causes links not to be clicked
when they're right at the edge of the screen (which was the case for the
notification appearing in the last place):
```
1) System Emails Preview Pending #moderate_pending
Failure/Error: click_on "Moderate notification send"
Selenium::WebDriver::Error::ElementNotInteractableError:
element not interactable: element has zero size
(Session info: headless chrome=91.0.4472.114)
```
