Just like we did in commits like f2e32b44b, a8537f7e1 and be9fc2265,
we're replacing a buggy JavaScript solution with one using just CSS.
Besides, we've had a failure in our test suite caused by an image not
being displayed on the page, with the message:
```
Failures:
1) Executions Images renders last milestone's image if investment has multiple milestones
with images associated
Failure/Error: expect(page).to have_css("img[alt='Second image']")
expected to find visible css "img[alt='Second image']" but there were no matches.
Also found "", which matched the selector but not all filters.
# ./spec/system/budgets/executions_spec.rb:135:in `block (3 levels) in <top (required)>'
```
The text "matched the selector but not all filters" means that the
element was present on the page but wasn't visible. One possible cause
is that the equalizer was adjusting the height of the element containing
the image before the image was loaded.
Note that, after these changes, all investments on the same row will
have the same height but, unlike with Foundation's equalizer,
investments on different rows might have different heights.
Note that we're changing the component so it uses `polymorphic_path`;
that way we don't have to pass the `@budget` variable to the component.
We could also use `budget_investment_path investment.budget, investment`
instead.
In commit 96ae69fe9, we stopped using cookies to track Ahoy visits and
started using a combination of the IP and the browser agent instead.
However, since we're still using the legacy logic from Ahoy 1.x to track
visits (which we had to add in commit b5220effd), this way of tracking
visits doesn't work and counts every page visited by a user as an
independent visit.
Maybe we could migrate existing data, which uses the `visitor_id` column
so it uses the new `visit_token` and `visitor_token` columns, but
there's no mention in the Ahoy documentation regarding how to do so.
While deciding what to do about this, we found something interesting.
For two years, we've been seeing random failures in the
`system/admin/tenants_spec.rb` tests, with messages like:
```
1) Tenants Create Tenant with subdomain
Failure/Error:
raise TenantNotFound, <<~EXCEPTION_MESSAGE
Could not set search path to schemas, they may be invalid:
"#{tenant}" #{full_search_path}.
Original error: #{exception.class}: #{exception}
EXCEPTION_MESSAGE
Apartment::TenantNotFound:
Could not set search path to schemas, they may be invalid:
"earth" "public", "shared_extensions".
Original error:
ActiveRecord::StatementInvalid: Could not find schema earth
```
And we've found one of the causes: the AJAX requests done by Ahoy to
track visits. Sometimes a test that creates or updates a tenant finishes
but the Ahoy AJAX request to, say, `earth.lvh.me/ahoy/visits`, is
handled by the next test, when the `earth` schema no longer exists, thus
raising an `Apartment::TenantNotFound` exception.
So by disabling these AJAX requests and tracking the visits in the
server instead, we're killing two birds in one stone: we're fixing the
bug regarding the visits count and we're reducing the flakiness in our
test suite. It looks like we're also removing the "phantom ahoy cookie"
we were getting since the mentioned commit b5220effd: an ahoy cookie was
quickly set and unset in the browser.
Note that, even though we aren't migrating any data, we're still adding
the new fields, because some tests started to fail because, when
tracking visits in the server without cookies, Ahoy expects the Visit
model to have a `visit_token` field.
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.
- 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.
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.
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.
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.
We don't need to replace the whole row, since the changes only affect
the button. Therefore, we don't need to depend on an `inserted` event to
decide which columns to render in that row.
We were checking it in the view, meaning that it was possible to toggle
the selection by sending a custom request even when the investment
wasn't feasible.
This way it'll be easier to change the link/button used to toggle the
selection.
Note that the conditions in the view seem to be different because we no
longer include the `selected?` condition when rendering the link/button.
However, an investment can only be selected if it's feasible and its
valuation is finished, so writing something like this would have been
redundant:
```ruby
can?(:toggle_selection, investment) &&
(selected? || investment.feasible? && investment.valuation_finished?)
```
The reason why the previous code was using the `selected?` condition was
to check whether to render the link/button to select or to deselect an
investment. We're now doing that in the Ruby part of the component.
Since we define the `data-field` element, we can style each element
individually with CSS.
I'm not sure whether these styles make sense, though. For instance, why
is "Supports" aligned to the center, since it's a number? For now, we're
leaving it as it was.
This code isn't used since commit c9f31b8e1.
Since we no longer depend on the content of the `#investments` element
being in a separate partial, we're also moving this element to the
partial itself and adding an HTML class to it, like we usually do.
We're also removing the code that loads all the investments in the
`toggle_selection` action, which wasn't needed since commit 3278b3572,
when we stopped rendering all the investments in this action.
This way we'll be able to simplify it a little bit.
Note that the original partial didn't include the whole row and only
the cells. Since, most of the time, we include the whole row in
partials, we're slightly modifying the component.
Since this button is replaced by a new element in an AJAX call, nothing
was focused after pressing it.
So we're reusing the code we used to enable/disable budget phases, which
already dealt with this issue.
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.
The button to select/deselect a proposal wasn't very intuitive; for
example, it wasn't obvious that pressing a button saying "selected"
would deselect the proposal.
So we're using a switch control, like we do to enable/disable features
since commit fabe97e50.
When clicking the browser's back button, browsers usually don't reload
the page but show a cached version of the page.
Turbolinks takes this one step further. When clicking on a link to a
page that's already cached, turbolinks displays the cached version of
the page and then it reloads it.
I don't really like this behavior but, since it affects the whole
application and we're about to release a patch version :), for now we're
keeping it this way in the development and production environments.
In the test environment, however, we're disabling these previews because
they might lead to requests leaking between tests.
For example, a test that visits the investments index, then goes to
"check my votes", then clicks on "Go back" and finishes by checking some
content on this page will result in those checks being done against the
cached version of the page. If these checks pass before turbolinks
reloads the page, the "Go back" request will finish during the test that
runs immediately after this one, resulting in unpredictable results.
Disabling the previews solves the issue.
Since the PR "Do not use third-party cookies in embedded videos #5548", the logic from
"embed_videos_helper" was extracted to the "embedded_video_component" and the
"videoable" model concern.
However, during this refactor, the "regex" method, which uses record.class:: to handle
video embeds, was left inaccessible for Legislation Proposals.
This commit fixes the issue by including the concern in the Legislation Proposal model.
