We're changing the order of the elements in the HTML so the menu button
appears next to the menu it opens, with no logo between them, which IMHO
makes sense and makes it easier to understand the layout for people
using screen readers.
A small advantage of this approach is that on very narrow screens or
Consul applications having a very long word for "Menu", the menu button
appeared on top, the logo appeared below it, and the contents of the
menu appeared below the logo. Now the logo appears on top, the menu
button appears below it, and the contents of the menu appear below the
menu button.
The menu didn't look properly on these screens since commit dcec003d0.
On small screens with enough horizontal space to show the menu button,
the logo, and the contents of the menu, all three elements were shown on
the same row, which looked broken.
Now the contents of the menu are shown below the menu button.
Note that, to force this, we're making the contents of the menu 100%
wide. That means links would take the 100% of the space, which would
make it easy to click on a link while trying to scroll when using
touchscreens. So we're making the links as wide as their text, which
also has a disadvantage: it's harder to click on narrow links like
"SDG".
This class was only there in order to use Foundation's styles, but
the amount of styles we were using was equal to the amount of
styles we were overwriting.
We're trying to be consistent; in the past, we had the partials
"shared/admin_login_items", "layouts/notification_item" and
"devise/menu/login_items". Now we're moving all these partials to
components in the `Layout` namespace.
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'll be easier to decide when they should be rendered.
In order to be consistent, we're using the `Layout` module for both
components; previously, the navigation partial was in the `shared`
folder while the footer partial was in the `layout` folder, which IMHO
didn't make much sense.
This way we remove the duplication in the layouts which had these links.
Since we're now passing the `current_user` option to partials in all
cases, IMHO the code is easier to follow if we use the
`Layout::NotificationItemComponent` instead of its partial.
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/
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.
We were using similar code in four different places; six, if we count
the welcome pages seeds. Reducing duplication in the pages seeds is a
bit tricky because administrators are supposed to edit their content and
might remove the HTML class we use to define styles. However, we can
share the code everywhere else.
Note that there's a bug in the application since we show that level 2
users cannot vote for budget projects but we give them permission to do
so in the abilities model. We're keeping the same behavior after this
refactoring but we might change it in the future.
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.
We were using the same logic in many different places, so we're
simplifying the code. I'm not convinced about the method names, though,
so we might change them in the future.
Note using this method for the default tenant in the `TenantDiskService`
class resulted in a `//` in the path, which is probably harmless but
very ugly and it also generates a different key than the one we got
until now. I've added an extra test to make sure that isn't the case.
We forgot to do so in commit d827768c0. In order to avoid the same
mistake in the future, we're extracting a method to get these
attributes. We're also adding tests, since we didn't have any tests to
check that the `dir` attribute was properly set.
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.
While we ping some search engines (currently, only Google) when
generating the sitemap files, we weren't telling search engines
accessing through the `robots.txt` file where to find the sitemap. Now
we're doing so, using the right sitemap file for the right tenant.
The `reload` method added to max_votes validation is needed because the
author gets here with some changes because of the around_action
`switch_locale`, which adds some changes to the current user record and
therefore, the lock method raises an exception when trying to lock it
requiring us to save or discard those record changes.
We've been using the `url` Setting for a long time, but since then we've
added a few references to `root_url` to this file, so we're now adding
consistency. We're also removing a now unnecessary condition.
We were using `Setting["url"]` to verify the content belonged to the
application URL, but we can use `root_url` instead.
Note that means we need to include the port when filling in forms in the
tests, since in tests URL helpers like `polymorphic_url` don't include
the port, but a port is automatically added when actually making the
request.
This task was "temporarily" removed in commit 7b6619528. Since that was
done three and a half years ago, right after the dashboard was
introduced, I think it's time to make this "temporary" measure a bit
more permanent ;).
By using the Rails `button_to` helper (which generates a form), and adapting the
response to `html` and `js` formats, the feature works with or without javascript
enabled.
Note that the `create` action doesn't create an image but updates an
answer instead. We're removing the references to `:create` in the
abilities since it isn't used.
In the future we might change the form to add an image to an answer
because it's been broken for ages since it shows all the attached
images.
Adding, modifiying, and/or deleting questions for an already started
poll is far away from being democratic and can lead to unwanted side
effects like missing votes in the results or stats.
So, from now on, only modifiying questions will be possible only if
the poll has not started yet.
In this form, the only case where `poll` might be present without
`question.poll` being present to is going to be the `new` action. We can
assign the poll in the `new` action and get rid of the `poll` variable
in the form.
