Currently in the application we never show the original image, we always
show one of its variants.
This change removes the metadata of both the variants and the original
version so that if at some point we decide to show the original version,
we will no longer have to remember to remove the metadata.
Note that in the budgets wizard test we now create district with no
associated geozone, so the text "all city" will appear in the districts
table too, meaning we can't use `within "section", text: "All city" do`
anymore since it would result in an ambiguous match.
Co-Authored-By: Julian Herrero <microweb10@gmail.com>
Co-Authored-By: Javi Martín <javim@elretirao.net>
We were using `map_location` in one place and
`location-map-remove-marker` in another one. We usually use dashes in
HTML class names, we don't say "location map" anywhere else.
We were rendering the whole sidebar again, which wasn't necessary since
most of it remains unchanged. This resulted in complicated code to pass
the necessary information to render the same map that was already
rendered. Furthermore, when users had been moving the map around or
zooming out, we were resetting the map to its default settings after
voting, which was potentially annoying.
This also fixes the wrong investments being displayed in the map after
voting; only the investments on the current page were displayed in this
case while the index displayed all of them.
Since this test was added in commit 78c6a30424 the
"current_password" was filled with an incorrect value.
This test did not fail because in previous versions of the
"devise-security" gem if this "current_password" was not valid
a "self.valid?" was performed which caused the error we are testing.
In version 0.17 (as can be seen in the following commit
https://github.com/devise-security/devise-security/pull/340/commits/41a99b67fe0)
if the "current_password" is not valid, related errors are still added
but the "self.valid?" code is removed, which is what causes the
expected error "must be different than the current password"
to appear in the "new password" field.
By adding a correct "current_password" in the test, we avoid this validation
for the "current_password" (which no longer includes the expected error)
and follow the natural flow of devise that does end up intercepting the error
for the new password entered.
The test that was not passing sometimes since commit 915999db4 because
we no longer created the budget before visiting the page that loads it.
So now we're forcing its creation with `let!`.
This is consistent with the component for balloting stats. We're about
to change both components, and the changes are easier to follow if
they're similar.
We're also using consistent names in methods.
We're also moving the tests, but we're keeping one system test in order
to test the controller and the navigation to get to this page.
Note we're slightly changing the order of the methods in the component;
the order of the instance variables was `user_`, `vote_`, `vote_`,
`user_`, which was hard to follow.
We don't need groups to create headings since commit 20b1085dc, and
`let!` was used as a substitute for instance variables in commit
91c21b098, but it isn't necessary in this case.
Since the change on commit cbbe188d6 we added a Poll.current.any?
condition to show the officing link on admin menu to officers.
That condition doesn't have much sense since Poll results only can be
added after a poll has ended, and there may be only one active poll.
In most sections, we had two specs testing what happens after accessing
one of the privileged areas. We're grouping the expectations and so
we've only got one test per area, making these tests faster.
This way we reduce the number of system tests or, in some cases,
requests during system tests, making the tests faster.
We're still testing the interaction with the menu when users have the
right permissions.
Since we allow many active budgets at the same time, the
controller should now check the budget given by params.
Before this change the controller was checking the latest
published budget, ignoring the request parameter `budget_id`.
We were getting a warning with Ruby 2.7:
```
ruby/gems/2.7.0/gems/capybara-3.37.1/lib/capybara/session.rb:377:
warning: Using the last argument as keyword parameters is deprecated;
maybe ** should be added to the call
```
On Ruby 3.0, the test failed with `Unable to find fieldset
{:text=>"Draft phase"}` and we were also getting another warning:
```
Locator Hash:{:text=>"Draft phase"} for selector :fieldset must be an
instance of String or Symbol. This will raise an error in a future
version of Capybara
```
We were getting a warning with Ruby 2.7 due to the change in the way
keyword arguments are handled in Ruby 3.0.
```
ruby/gems/2.7.0/gems/rspec-support-3.11.0/lib/rspec/support/with_keywords_when_needed.rb:18:
warning: Passing the keyword argument as the last hash parameter is
deprecated
```
As hinted by the warning, this code crashes with Ruby 3.0:
```
ArgumentError:
unknown keyword: :budget_id
```
I'm not sure why this is the case, though, since we were already
explicitely passing a hash first before passing the keyword parameters.
I guess there are some cases in this whole keyword VS hash parameters
incompatibility that I haven't completely understood.
In the management section, `current_user` is the user impersonated by
the manager. We were deciding whether to show the admin menu depending
on the privileges of the current user, but this menu should be shown
according to the privileges of the manager who is impersonating the
user.
We're doing a similar (very subtle) change in the login items. We were
rendering the `login_items` partial passing `current_user: user`.
However, inside this method, we were using `user_signed_in`, which
ignored the `current_user` we were passing. The result was always the
same expect in tests where we manually sign in users, but we're changing
it anyway in order to reduce confusion.
This way it's easier to refactor it.
Note we're using `with_request_url` in the tests because the component
renders the locale switcher, which needs a URL in order to work. This
doesn't affect whether we're in the management section or not.
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/
In general, we don't use links inside admin tables because we don't know
where the link will point to, and use "view" actions/links instead.
However, in this case, we're showing a URL, so it's perfectly obvious
where the link will point to. And so it makes sense to use the URL as a
link instead of using a "view" action/link.
Some institutions using CONSUL have expressed interest in this feature
since some of their tenants might already have their own domains.
We've considered many options for the user interface to select whether
we're using a subdomain or a domain, like having two separate fields,
using a check box, ... In the end we've chosen radio buttons because
they make it easier to follow a logical sequence: first you decide
whether you're introducing a domain or subdomain, and then you enter it.
We've also considered hiding this option and assuming "if it's got a
dot, it's a domain". However, this wouldn't work with nested subdomains
and it wouldn't work with domains which are simply machine names.
Note that a group of radio buttons (or check boxes) is difficult to
style when the text of the label might expand over more than one line
(as is the case here on small screens); in this case, most solutions
result in the second line of the label appearing immediately under the
radio button, instead of being aligned with the first line of the label.
That's why I've added a container for the input+label combination.
Just like we respond with "not found" for any other record. This
improves the user experience because with the "Not found" error message
people realize the URL is wrong instead of thinking that they broke the
application.
While creating the "venus" tenant for every test makes the code cleaner,
it also makes the tests much slower, so we aren't doing so in tests
where we don't use this tenant.
One of these tests has failed once because there wasn't a user with the
right confirmation token. While I haven't been able to reproduce the
issue, there's a chance it's caused by a `visit` call to the
confirmation path which might start before the redirect request to the
successful sign up page has finished.
I'm not sure this is the case, though, but, worst case scenario, if the
test fails again we'll know it isn't because of a missing expectation.
When voting investment projects, the sidebar was rendered without the
`@heading_content_blocks` being set. That resulted in a 500 error when
the heading had content blocks.
By extracting the logic to a component, we make sure the heading content
blocks are properly set every time this code is rendered, no matter
which controller is rendering the view.
This is something we had read about a long time ago, but didn't find how
to reproduce the issue until now.
As mentioned in the Apartment documentation:
> it's important to consider that you may want to maintain the
> "selected" tenant through different parts of the Rack application
> stack. For example, the Devise gem adds the Warden::Manager middleware
> at the end of the stack in the examples above, our
> Apartment::Elevators::Subdomain middleware would come after it.
> Trouble is, Apartment resets the selected tenant after the request is
> finished, so some redirects (e.g. authentication) in Devise will be
> run in the context of the "public" tenant. The same issue would also
> effect a gem such as the better_errors gem which inserts a middleware
> quite early in the Rails middleware stack.
>
> To resolve this issue, consider adding the Apartment middleware at a
> location in the Rack stack that makes sense for your needs, e.g.:
>
> Rails.application.config.middleware.insert_before Warden::Manager,
> Apartment::Elevators::Subdomain
>
> Now work done in the Warden middleware is wrapped in the
> Apartment::Tenant.switch context started in the Generic elevator.
We're using the "tenants" subfolder for consistency with the folder
structure we use in ActiveStorage and because some CONSUL installations
might have folders inside the `data` folder which might conflict with
the folders created by tenants.
Note that the Python scripts have a lot of duplication, meaning we need
to change all of them. I'm not refactoring them because I'm not familiar
enough with these scripts (or with Python, for that matter).
Also note that the scripts folder is still shared by all tenants,
meaning it isn't possible to have different scripts for different
tenants. I'm not sure how this situation should be handled; again, I'm
not familiar enough with this feature.
