Our original interface to vote in a poll had a few issues:
* Since there was no button to send the form, it wasn't clear that
selecting an option would automatically store it in the database.
* The interface was almost identical for single-choice questions and
multiple-choice questions, which made it hard to know which type of
question we were answering.
* Adding other type of questions, like open answers, was hard since we
would have to add a different submit button for each answer.
So we're now using radio buttons for single-choice questions and
checkboxes for multiple-choice questions, which are the native controls
designed for these purposes, and a button to send the whole form.
Since we don't have a database table for poll ballots like we have for
budget ballots, we're adding a new `Poll::WebVote` model to manage poll
ballots. We're using WebVote instead of Ballot or Vote because they
could be mistaken with other vote classes.
Note that browsers don't allow removing answers with radio buttons, so
once somebody has voted in a single-choice question, they can't remove
the vote unless they manually edit their HTML. This is the same behavior
we had before commit 7df0e9a96.
As mentioned in c2010f975, we're now adding the `ChangeByZero` rubocop
rule, since we've removed the test that used `and change`.
We removed the link to this page in commit 83e8d6035 because poll
questions don't really make sense without a poll.
However, this page also contained information about successful
proposals, which might be interesting so administrators don't have to
navigate to the public area in order to find and create questions based
on successful proposals.
So we're keeping the part about successful proposals and linking it from
the proposals part of the admin area.
Note we're using translation keys like `successful_proposals_tab`, which
don't make sense anymore, for the successful proposals. We're doing so
because we've already got translations for these keys and, if we renamed
them, we'd lose the existing translations and our translators would have
to add them again.
Also note we're changing one poll question test a little bit so we
create the question from a successful proposal using the new page. There
are other tests checking how to create a question from the
admin/proposals#show action and other tests checking what happens when
accessing a successful proposal in the admin section, so we don't lose
any test coverage by changing an existing test instead of adding a new
one.
Finally, note that we've removing the `search` method in poll question
because we no longer use it. This currently makes the
`author_visible_name` database column useless; we aren't removing it
right now because we don't want to risk a possible data loss in a patch
release (we're about to release version 2.3.1), but we might remove it
in the future.
We only want to render the account link and login items in the header.
And we want only render the Multitenancy and Administrators sections in
the admin sidebar.
We include the administrators management so it's possible to give
permissions to other users to manage tenants.
In order to restrict access to other sections by typing the URL or
following a link, we're only enabling the rest of the routes when we
aren't in the multitenancy management mode.
We're going to add some constraints in the routes file, and if we add a
`resolve` clause inside a constraints block, we get an error saying that
"The resolve method can't be used inside a routes scope block" when
starting the application.
Using a checkbox wasn't very intuitive because checkboxes are
checked/unchecked when clicked on even if there's an error in the
request. Usually, when checkboxes appear on a form, they don't send any
information to the server unless we click a button to send the form.
So we're using a switch instead of a checkbox, like we did to
enable/disable phases in commit 46d8bc4f0.
Note that, since we've got two switches that match the default
`dom_id(record) .toggle-switch` selector, we need to find a way to
differentiate them. We're adding the `form_class` option for that.
Also note that we're now using a separate action and removing the
JavaScript in the `update` action which assumed that AJAX requests to
this action were always related to updating the `visible_to_valuators`
attribute.
This is consistent to what we usually do. Also, we're applying the same
criteria mentioned in commit 72704d776:
> We're also making these actions idempotent, so sending many requests
> to the same action will get the same result, which wasn't the case
> with the `toggle` action. Although it's a low probability case, the
> `toggle` action could result in [selecting an investment] when trying
> to [deselect] it if someone else has [deselected it] it between the
> time the page loaded and the time the admin clicked on the
> "[Selected]" button.
This is consistent to what we usually do. Also, we're applying the same
criteria mentioned in commit 72704d776:
> We're also making these actions idempotent, so sending many requests
> to the same action will get the same result, which wasn't the case
> with the `toggle` action. Although it's a low probability case, the
> `toggle` action could result in [selecting a proposal] when trying to
> [deselect] it if someone else has [deselected it] it between the time
> the page loaded and the time the admin clicked on the "[Selected]"
> button.
Until now, people had to edit the original route files in order to add
custom routes.
This was inconsistent with the other customizations, since we use custom
folders or files for customizing controllers, components, views, ...
(which you usually customize as well when adding a new route).
So now we're providing a file for custom routes, which will make it
easier to know which routes are not present in Consul Democracy by
default.
It was confusing to have the action to create an answer in
`QuestionsController#answer` while the action to destroy it was
`AnswersController#destroy`.
The routes for poll questions were accidentally deleted in commit
5bb831e959 when deleting the `:show` action, and restored in commit
9871503c5e. However, the deleted code was:
```
resources :questions, only: [:show], controller: 'polls/questions' (...)
```
While the restored code was:
```
resources :questions, controller: 'polls/questions' (...)
```
Meaning we forgot to add the `only: []` option when restoring the
routes.
We also forgot to remove the `before_action` code when deleting the
`:show` action, so we're removing it now.
Having a class named `Poll::Question::Answer` and another class named
`Poll::Answer` was so confusing that no developer working on the project
has ever been capable of remembering which is which for more than a few
seconds.
Furthermore, we're planning to add open answers to polls, and we might
add a reference from the `poll_answers` table to the
`poll_question_answers` table to property differentiate between open
answers and closed answers. Having yet another thing named answer would
be more than what our brains can handle (we know it because we did this
once in a prototype).
So we're renaming `Poll::Question::Answer` to `Poll::Question::Option`.
Hopefully that'll make it easier to remember. The name is also (more or
less) consistent with the `Legislation::QuestionOption` class, which is
similar.
We aren't changing the table or columns names for now in order to avoid
possible issues when upgrading (old code running with the new database
tables/columns after running the migrations but before deployment has
finished, for instance). We might do it in the future.
I've tried not to change the internationalization keys either so
existing translations would still be valid. However, since we have to
change the keys in `activerecord.yml` so methods like
`human_attribute_name` keep working, I'm also changing them in places
where similar keys were used (like `poll_question_answer` or
`poll/question/answer`).
Note that it isn't clear whether we should use `option` or
`question_option` in some cases. In order to keep things simple, we're
using `option` where we were using `answer` and `question_option` where
we were using `question_answer`.
Also note we're adding tests for the admin menu component, since at
first I forgot to change the `answers` reference there and all tests
passed.
We're using different controls depending on the number of available
locales.
When there are only a few locales, the solution is obvious: radio
buttons to select the default language, and checkboxes to select the
available ones are simple and intuitive.
With many languages, showing two consecutive lists of 30 languages could
be confusing, though, particularly on small devices, where scrolling
through both lists could be hard.
So, in this case, we're rendering a <select> to choose the default
language. For selecting the available languages, however, we're sticking
to checkboxes because all the other existing options (like multiple
selects) are hard to use. We think it's OK because the form doesn't have
any additional fields, and there's only one big list of options to
scroll through.
While testing the application, we noticed that if we use the
`admin-fieldset-separator` styles when there's only one fieldset, it's
harder to notice that there's an additional field to select the default
language. So we're only using the `admin-fieldset-separator` styles when
all the fields are grouped in fieldsets.
Regarding the help text for the fieldset, if we leave the help text
outside the <legend> tag, people using screen readers won't hear about
this content. However, if we include it inside the <legend> tag, some
screen readers might read it every time they move to a different
checkbox (or radio button), which can be annoying. Since I don't think
these help messages are really essential, I'm leaving them out of the
<legend> tag. It's also easier to style them if they're outside the
<legend> tag.
Note we're using `display: table` for the labels, for the reasons
mentioned in commit 923c2a7ee.
Also note that, when there's only one available locale, this section is
useless. In this case, we aren't disabling it for now because there's a
chance people see it in the official Consul Democracy demo and then
wonder why it isn't available on their installation. We might disable it
in the future, though.
Now that we've moved the logic to generate the events data to a model,
and we've got access to the model in the component rendering the chart,
we can render the data inside the chart instead of doing an extra AJAX
request to get the same data.
Originally, this was problaby done this way so the page wouldn't take
several seconds to load while preparing the data for the chart when
there are thousands of dates being displayed. With an AJAX call, the
page would load as fast as usual, and then the chart would render after
a few seconds. However, we can have an even better performance
improvement in this scenario if we use a Set instead of an Array. The
method `Array#include?`, which we were calling for every date in the
data, is much slower that `Set#merge`. So now both the page and the
chart load as fast as expected.
We could also use something like:
```
def add
(...)
shared_keys.push(*collection.keys)
end
def build
(...)
shared_keys.uniq.each do |k|
(...)
end
def shared_keys
@shared_keys ||= []
end
```
Or other approaches to avoid using `Array#include?`. The performance
would be similar to the one we get when using `Set`. We're using a `Set`
because it makes more obvious that `shared_keys` is supposed to contain
unique elements.
We've had some tests failing in the past due to these AJAX requests
being triggered automatically during the tests and no expectations
checking the requests have finished, so now we're reducing the amount of
flaky tests.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
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).
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
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.
By using the bindPopup function instead of the click event
popups work when using the keyboard.
Note that now we are loading all the map markers in the first
request in a single query to the database (needed when there
is a lot or markers to show). Because of that we removed the
AJAX endpoint.
The only view that linked to this action was never used and so it was
deleted in commit 0bacd5baf.
Since now the proposals controller is the only one place rendering the
`shared/map` partial, we're moving it to the proposals views.
Note we could use `acts_as_paranoid` with the `without_default_scope`
option, but we aren't doing so because it isn't possible to consider
deleted records in uniqueness validations with the paranoia gem [1].
I've added tests for these cases so we don't accidentally add
`acts_as_paranoid` in the future.
Also note we're extracting a `RowComponent` because, when
enabling/disabling a tenant, we're also enabling/disabling the link
pointing to its URL, and so we need to update the URL column after the
AJAX call.
[1] See issues 285 and 319 in https://github.com/rubysherpas/paranoia/
This is consistent with the way we use separate actions to hide and
restore records, which is similar to enabling and disabling a record. We
might do something similar with the `toggle_selection` actions in the
future. For now, we're only doing it with budget phases because we're
going to add a similar switch control to hide and restore tenants.
We're also making these actions idempotent, so sending many requests to
the same action will get the same result, which wasn't the case with the
`toggle` action. Although it's a low probability case, the `toggle`
action could result in disabling a phase when trying to enable it if
someone else has enabled it between the time the page loaded and the
time the admin clicked on the "enable" button.
Note we aren't allowing to delete a tenant because it would delete all
its data, so this action is a very dangerous one. We might need to add a
warning when creating a tenant, indicating the tenant cannot be
destroyed. We can also add an action to delete a tenant which forces the
admin to write the name of the tenant before deleting it and with a big
warning about the danger of this operation.
For now, we're letting administrators of the "main" (default) tenant to
create other tenants. However, we're only allowing to manage tenants
when the multitenancy configuration option is enabled. This way the
interface won't get in the way on single-tenant applications.
We've thought about creating a new role to manage tenants or a new URL
out of the admin area. We aren't doing so for simplicity purposes and
because we want to keep CONSUL working the same way it has for
single-tenant installations, but we might change it in the future.
There's also the fact that by default we create one user with a known
password, and if by default we create a new role and a new user to
handle tenants, the chances of people forgetting to change the password
of one of these users increases dramatically, particularly if they
aren't using multitenancy.
These routes aren't used since commits (2993ef8707, 88a7a29d27)
In parallel while these routes were being removed, the route file
was being split into different files, and they were included again
in the commit 1cd47da9d4.
Until now, in order to edit an answer, we had to click on its title on
the table and then on the "Edit answer" link.
That was tedious and different from what we usually do in the admin
section. Furthermore, the code for the answers table was written twice
and when we modified it we forgot to update the one in the `show`
action, meaning the table here provided less information than the
information present in the answers tables.
Co-Authored-By: Javi Martín <javim@elretirao.net>
The current consul GraphQL API has two problems.
1) It uses some unnecessary complicated magic to automatically create
the GraphQL types and querys using an `api.yml` file. This approach
is over-engineered, complex and has no benefits. It's just harder to
understand the code for people which are not familiar with the
project (like me, lol).
2) It uses a deprecated DSL [1] that is soon going to be removed from
`graphql-ruby` completely. We are already seeing deprecation warning
because of this (see References).
There was one problem. I wanted to create the API so that it is fully
backwards compatible with the old one, BUT the old one uses field names
which are directly derived from the ruby code, which results in
snake_case field names - not the GraphQL way. When I'm using the
graphql-ruby Class-based syntax, it automatically creates the fields in
camelCase, which breaks backwards-compatibility.
So I've added deprecated snake_case field names to keep it
backwards-compatible.
[1] https://graphql-ruby.org/schema/class_based_api.html
The map feature was never implemented for debates (only for proposals
and budget investments) and it was crashing for debates because the page
didn't load the geozones. And we don't have a "geozone" field in the
debates form either.
So we're removing the map page alongside its (pending implementation)
tests.
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.
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.
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 `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
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.
