The "on_budget_investments" scope in Activity has never been used
anywhere in the codebase. It was introduced in commit d9d38482b3
("extends Activity to include Investment valuations") but no references
were ever added.
Instead of removing it, we make use of the scope by adding the missing
"Budget investments" filter to the admin Activity section. This aligns
it with the rest of the activity filters and gives the scope the purpose
it was originally intended for.
The "sort_by_most_commented" scope in Debate is no longer used anywhere in
the code. Its last use was removed in commit b89f39bfef ("Removes
unused orders from debates controller")
We were calling `parse_remote_to_hash` in the Devise initializer, which
runs when the application starts.
That meant that, if we got an exception when calling that method, the
application wouldn't start. We got exceptions if the single sign-on
(SSO) URL isn't available or we aren't providing the right credentials.
So we're moving the call to `parse_remote_to_hash` to
`OmniauthTenantSetup`, which is only called when actually trying to sign
in with SAML.
Since we're moving the code there, we're also unifying the code so SAML
settings are configured the same way for the main tenant and other
tenants, like we did for OpenID Connect in commit c3b523290.
In order to keep the existing behavior, we're caching the result of
`parse_remote_to_hash` in an instance variable. Not sure about the
advantages and disadvantages of doing so over parsing the remote URL
metadata on every SAML-related request.
Note that the SAML tests in `OmniauthTenantSetup` use the `stub_secrets`
method. But this method is called after the application has started,
meaning it doesn't stub calls to `Rails.application.secrets` in
`config/initializers/`. So, before this commit, the code that parsed the
IDP metadata URL wasn't executed in the tests. Since now we've moved the
code but we don't want to depend on external URLs when running the
tests, we need to stub the call to the external URL. Since we're now
stubbing the call, we're adding expectations in the tests to check that
we correctly use the settings returned in that call.
The `issuer` setting was renamed to `sp_entity_id` in omniauth-saml [1],
and it's been deprecated in ruby-saml since version 1.11.0, released on
July 24, 2019 [2].
The ruby-saml code currently uses:
```
def sp_entity_id
@sp_entity_id || @issuer
end
```
So setting `issuer` to the same value as `sp_entity_id` if
`sp_entity_id` is present, as we were doing, has no effect.
On the other hand, neither omniauth-saml nor ruby-saml use the
`idp_metadata_url` and `idp_metadata` settings.
[1] https://github.com/omniauth/omniauth-saml/commit/74ed8dfb3aed
[2] https://github.com/SAML-Toolkits/ruby-saml/releases/tag/v1.11.0
When we first added OIDC support, we were configuring the redirect URI
in the devise initializer, just like we did for other providers.
Thanks to the changes in the previous commit, that code is no longer in
the devise initializer, which means we can use `url_helpers` to get the
redirect URI.
This means we no longer need to define this URI in the secrets. This is
particularly useful for multitenancy; previously, we had to define the
redirect URI for every tenant because different tenants use different
domains or different subdomains.
We were using the `client_options` hash for the default tenant, defined
in the Devise initializer, but we forgot to include that key in the
multitenant code. This means OIDC wasn't working when different tenants
used different configurations.
Note that we are not including Poll::PartialResults for open-ended
questions resutls. The reason is that we do not contemplate the
possibility of there being open questions in booths. Manually
counting and introducing the votes in the system is not feasible.
Running tests at the component level is faster than at the system level,
so we move tests from system/polls/results_spec.rb to the component.
Note that moving these tests removes vote_for_poll_via_web and the visit
to results_poll_path, but both are already covered in other tests. We
also take the opportunity to reuse the method in another test where
it makes sense.
Additionally, the spec title has been reverted from "Results for polls
with questions but without options" to "renders results for polls with
questions but without answers", as it was before commit 8997ed316c.
The "Maximum number of votes" text in poll question show was unnecessary.
It appeared for both unique and open-ended questions, but it only makes
sense for questions that allow multiple answers.
Just as we mentioned in the previous commit, there are places where we
aren't sure whether explicit consent is strictly required. So, when the
"require consent" setting is enabled, we're taking the safe approach.
This means that, in this case, we're only displaying a user's activity
if they've given explicit consent.
The GDPR is open for interpretation, and it isn't clear whether showing
users recommended proposals and debates while browsing the site is
considered a notification that needs to be explicitly accepted.
Since we aren't sure whether this is necessary, we're taking the safe
approach and disabling recommendations by default.
Ensure GDPR compliance by default (Article 25 GDPR – privacy by design
and by default). Under GDPR, consent must be freely given, specific,
informed and unambiguous [1]. We were subscribing users without
explicity consent, which goes against the "No pre-ticked boxes"
principle.
For compatibility with existing installations, we're using a setting,
disabled by default. Once we release version 2.4.0 we will enable it by
default, which won't affect existing installations but only new ones.
[1] https://gdprinfo.eu/best-gdpr-newsletter-consent-examples-a-complete-guide-to-compliant-email-marketing
Adds rake task "polls:remove_duplicate_partial_results" to delete duplicated
rows in "poll_partial_results" made before the DB was strict about duplicates.
Duplicates are considered only for records without "option_id", grouping by:
(question_id, booth_assignment_id, date, answer). We keep the first one and
delete the rest, per tenant.
The controller use:
Poll::PartialResult.find_or_initialize_by(booth_assignment_id, date, question_id, answer)
which is not a strong protection against race conditions. Without a unique
index at the DB level, duplicates could be created. This task cleans up any
existing duplicates.
Similar to what we did in PR "Avoid duplicate records in poll answers" 5539,
specifically in commit 503369166, we want to stop relying on the plain text
"answer" and start using "option_id" to avoid issues with counts across
translations and to add consistency to the poll_partial_results table.
Note that we also moved the `possible_answers` method from Poll::Question to
Poll::Question::Option, since the list of valid answers really comes from the
options of a question and not from the question itself. Tests were updated
to validate answers against the translations of the assigned option.
Additionally, we renamed lambda parameters in validations to improve clarity.
Note that we have the same code in the officing section.
Then we can use the same component.
Note also that we are removing the parts of the system specs that are now
covered by the component itself, and taking the chance to unify tests.
In these removals and unifications we take into account that there are
other specs which already cover user interaction in this section.
- name: :oidc → Identifier for this login provider in the app.
- scope: [:openid, :email, :profile] → Tells the provider we want the user’s ID (openid), their email, and basic profile info (name, picture, etc.).
- response_type: :code → Uses Authorization Code Flow, which is more secure because tokens are not exposed in the URL.
- issuer: Rails.application.secrets.oidc_issuer → The base URL of the OIDC provider (e.g., Auth0). Used to find its config.
- discovery: true → Automatically fetches the provider’s endpoints from its discovery document instead of manually setting them.
- client_auth_method: :basic → Sends client ID and secret using HTTP Basic Auth when exchanging the code for tokens.
Add system tests for OIDC Auth
Edit the oauth docs to support OIDC auth
Note that Rails 7.1 changes `find_or_create_by!` so it calls
`create_or_find_by!` when no record is found, meaning we'll rarely get
`RecordNotUnique` exceptions when using this method during a race
condition.
Adding this index means we need to remove the uniqueness validation.
According to the `create_or_find_by` documentation [1]:
> Columns with unique database constraints should not have uniqueness
> validations defined, otherwise create will fail due to validation
> errors and find_by will never be called.
We're adding a test that checks what happens when using
`create_or_find_by!`.
Note that we're creating voters combining `create_with` with
`find_or_create_by!`. Using `find_or_create_by!(...)` with all
attributes (including non-key ones like `origin`) fails when a voter
already exists with different values, e.g. an existing `origin: "web"`
and an incoming `"booth"`. In this situation the existing record is not
matched and the unique index raises an exception.
`create_with(...).find_or_create_by!(user: ..., poll: ...)` searches by
the unique key only and applies the extra attributes only on creation.
Existing voters are returned unchanged, which is the intended behavior.
For the `take_votes_from` method, we're handling a (highly unlikely, but
theoretically possible) scenario where a user votes at the same time as
taking voters from another user. For that, we're doing something similar
to what `create_or_find_by!` does: we're updating the `user_id` column
inside a new transaction (using a new transactions avoids a
`PG::InFailedSqlTransaction` exception when there are duplicate
records), and deleting the existing voter when we get a
`RecordNotUnique` exception.
On `Poll::WebVote` we're simply raising an exception when there's
already a user who's voted via booth, because the `Poll::WebVote#update`
method should never be called in this case.
We still need to use `with_lock` in `Poll::WebVote`, but not due to
duplicate voters (`find_or_create_by!` method will now handle the unique
record scenario, even in the case of simultaneous transactions), but
because we use a uniqueness validation in `Poll::Answer`; this
validation would cause an error in simultaneous transactions.
[1] https://api.rubyonrails.org/v7.1/classes/ActiveRecord/Relation.html#method-i-create_or_find_by
For the longest time, we've disabled the buttons to vote via web when
people had already voted in a booth. However, we were still allowing
HTTP requests to the actions to vote via web.
So we're adding a condition to prevent it.
The reason why we're changing the controller instead of the abilities
model (which is what we usually do) is that there might be side-effects
to the change. For instance, in the `Polls::PollComponent` class,
there's an `elsif cannot?(:answer, poll)` condition which would have a
different behavior if we changed the abilities model.
This way we remove duplication in the HTML.
We're also adding a test checking what happens when users can vote in
order to test the `render?` method we've added.
This could be the case when JavaScript is disabled.
Note that, in `Poll/WebVote` we're calling `given_answers` inside a
transaction. Putting this code before the transaction resulted in a test
failing sometimes, probably because of a bug that might be possible to
reproduce by doing simultaneous requests.
With the old interface, there wasn't a clear way to send a blank ballot.
But now that we've got a form, there's an easy way: clicking on "Vote"
while leaving the form blank.
This way it'll be easier to refactor it.
Note there was a system test which tested both the callout and the form
when unverified users visit a poll. We've split this system test in two
component tests.
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`.
Until now, when writing `create(:poll_answer, option: option)`, the
`answer` field would take the title of a random option instead of taking
the title from the `option` variable.
So now, if we're given the option, the `answer` field will be taken from
the option itself.
Note that writing something like:
```
option { question.question_options.find_by(title: answer) }
answer { option.title }
```
Would create an infinite loop when creating a poll answer if we don't
pass the `option` and/or the `answer` attributes.
So, instead, we're making the `option` depend on the `answer` attribute
exclusively when we pass the `answer` attribute to the factory.
