For reasons that might or might not affect production installations, the
test checking simultaneous requests to create poll voters in the
officing voters controller wasn't behaving as expected.
The expected behavior, since commit 9a8bfac5b, is that the second
request reaching the `with_lock` part of the code waits for the first
request to finish and so this second request raises an
`ActiveRecord::RecordInvalid` exception when trying to save a voter with
the same poll and the same user as the first one.
However, 95% of the time that wasn't the case. Instead, when entering
the `@user.with_lock` block, the second request would replace its
`@voter` object with the `@voter` object saved in the same request, so
the second call to `save!` would succeed as it would simply update the
existing record.
This is a behavior that we could accept if it were consistent and
happened 100% of the time, but that isn't the case. 5% of the time, we
do get the `ActiveRecord::RecordInvalid` exception. So 5% of the time we
got a failure in the test:
```
1) Officing::VotersController POST create does not create two records
with two simultaneous requests
Failure/Error: @user.with_lock { @voter.save! }
ActiveRecord::RecordInvalid:
Validation failed: User User has already voted
# ./app/controllers/officing/voters_controller.rb:25:in `block in create'
# ./app/controllers/officing/voters_controller.rb:25:in `create'
# ./app/controllers/application_controller.rb:50:in `switch_locale'
# ./spec/controllers/officing/voters_controller_spec.rb:15:in `block (5 levels) in <top (required)>'
```
So we're changing the `with_lock` block so it includes the
initialization of the object. This way, we get the
`ActiveRecord::RecordInvalid` exception 100% of the time.
Note that in commit 9a8bfac5b we also rescued the
`ActionDispatch::IllegalStateError` exceptions. I'm not why we were
getting those exceptions when running the tests, and I'm not sure
whether we keep getting after these changes, but it doesn't really
matter. The reason is that in Consul Democracy 2.3.0 we're going to add
a unique index to the `poll_voters` table, which (according to the tests
done in the past) will make both the `@user.lock` block and rescuing the
`ActionDispatch::IllegalStateError` unnecessary.
So, in other words, these changes will never make it to production
because this part of the code will be changed again before releasing
version 2.3.0.
One of these tests has failed in our CI with the following message:
```
1) Users Public activity user can hide public page
Failure/Error: expect(page).to have_content("activity list private")
expected to find text "activity list private" in "Language: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n Notifications\nYou are in\nMy content\nMy
account\nSign out\nDebates\nProposals\nVoting\nCollaborative
legislation\nParticipatory budgeting\nSDG\nHelp\nM\nManuela124\nUser has
no public activity\nOpen government\nThis portal uses the CONSUL
DEMOCRACY application which is open-source
software.\nParticipation\nDecide how to shape the city you want to live
in.\nCONSUL DEMOCRACY, 2024 Privacy Policy Terms and conditions of use
Accessibility"
```
Note how the text "User has no public activity" is present, which is a
message that appears when the user's activity is public.
A possible explanation is that we didn't check that the request done by
the "Save changes" button had finished before continuing with the tests.
Back when we wrote this test, submitting a form in a test would always
wait for the request to be finished before continuing, but a lot has
changed since then.
So we're adding an expectation to make sure the the changes have been
saved before making a new request.
We're also rearraging the blank lines in these tests and removing the
parenthesis in `have_content` expectations to be consistent with the
expectations we're adding.
After the previous commit, we were using `double` in many places but
were only using `instance_double` in one file. So, for consistency,
we're using `double` in this file as well.
Lately (not sure since when), from time to time we've been getting these
failures in our CI:
```
Failures:
1) CommentsHelper#comment_author_class returns is-author if author is the commenting user
Failure/Error: comment = instance_double(Comment, user_id: author_id)
the Comment class does not implement the instance method: user_id
# ./spec/helpers/comments_helper_spec.rb:48:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'
2) CommentsHelper#comment_author_class returns an empty string if commenter is not the author
Failure/Error: comment = instance_double(Comment, user_id: author_id - 1)
the Comment class does not implement the instance method: user_id
# ./spec/helpers/comments_helper_spec.rb:55:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:40:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:39:in `block (2 levels) in <top (required)>'
```
It might be related to the upgrade of rspec-rails done in commit
6fe222148 or maybe due to a change in github actions that caused some
tests to fail, as described in commits bedcb5bca and 3e44eeaee.
What might be causing the issue is the usage of `instance_double`
stubbing different methods in different tests (not sure this is the
cause, though).
We've seen that somebody got a similar error [1] (although it might not
have been for the same reason) and one of the maintainers of rspec-mocks
replied:
> I would recommend switching to double (as you mentioned) or
> refactoring to use something more defined.
So we're simply using `double`, which is what we usually use when
stubbing objects in the tests. Doing so is faster than further
investigating why the `instance_double` isn't reliable 100% of the time.
[1] See issue 1587 in https://github.com/rspec/rspec-mocks/
- Created `object_by_id_field` method in `BaseObject` to simplify the declaration of fields
with an `id` argument.
- Replaced all instances of `field ... do` blocks with `object_by_id_field` where fields require
an `id` argument across multiple types.
Note: Since we update to 1.80.1 deprecation warnings are appear when execute the assets:precompile command.
In order to silence this deprecation, we add silence_deprecation option in sass.rb initializer.
The code has also been updated to remove the deprecation warnings that appeared related to the function
darken(), lighten() and "Using / for division" instead of the function calc().
Bumps [sassc-embedded](https://github.com/sass-contrib/sassc-embedded-shim-ruby) from 1.70.1 to 1.80.1.
- [Commits](https://github.com/sass-contrib/sassc-embedded-shim-ruby/compare/v1.70.1...v1.80.1)
---
updated-dependencies:
- dependency-name: sassc-embedded
dependency-type: direct:production
update-type: version-update:semver-minor
...
Signed-off-by: dependabot[bot] <support@github.com>
When the `multitenancy_management_mode` is enabled.
In order to avoid infinite redirects when regular users try to access
the admin section, we're redirecting to the account page in this case.
Otherwise, the admin section would redirect to the root path, which
would redirect to the admin section, which would redirect to the root
path, and so on.
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.
- Introduced `run_graphql_field` in tests that focus on resolving specific fields, leveraging the method added in GraphQL 2.2.0.
- Continued using `execute` for broader cases where it is still necessary to test entire GraphQL queries.
There are many possible ways to implement this feature:
* Adding a custom middleware
* Using rack-attack with a blocklist
* Using routes constraints
We're choosing to use a controller concern with a redirect because it's
what we do to handle unauthorized cancancan exceptions.
Note: The parser error message format changed in GraphQL 2.2.0 due to the introduction
of a new optimized lexer and a hand-written parser (PR 4718). This commit updates
the `parser_error_raised?` method in the GraphqlController tests to correctly detect
errors using the new message format.
The previous pattern was checking for "Parse error on", but with the new version,
the error message now contains "Expected one of". This change ensures that the
tests for malformed queries continue to pass as expected.
Bumps [graphql](https://github.com/rmosolgo/graphql-ruby) from 2.0.31 to 2.3.18.
- [Release notes](https://github.com/rmosolgo/graphql-ruby/releases)
- [Changelog](https://github.com/rmosolgo/graphql-ruby/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rmosolgo/graphql-ruby/compare/v2.0.31...v2.3.18)
---
updated-dependencies:
- dependency-name: graphql
dependency-type: direct:production
update-type: version-update:semver-major
...
Signed-off-by: dependabot[bot] <support@github.com>
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.
We were performing 3 requests in order to refresh the page and check the
content was still there. We can use `refresh` instead.
We're also reusing the `investment1` variable in every test, instead of
redifining it in one of them.
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.
Just like it happened with proposals, the button to select/deselect an
investment wasn't very intuitive; for example, it wasn't obvious that
pressing a button saying "selected" would deselect the investment.
So we're using a switch control, like we do to enable/disable features
since commit fabe97e50.
Note that we're making the text of the switch smaller than in other
places because the text in the investments table it is also smaller
(we're using `font-size: inherit` for that purpose). That made the
button look weird because we were using rems instead of ems for the
width of the button, so we're adjusting that as well.
Also note we're changing the width of the switch to `6em` instead of
`6.25em` (which would be 100px if 1em is 16px). We're doing so because
we used 100 for the minimum width because it's a round number, so
now we're using another round number.
