When editing/showing a proposal or an investment, the most relevant
information regarding the marker are the coordinates. The title of the
proposal or investment is redundant because we already know the marker
is about that proposal/investment.
There's one problem with this approach, though: when editing a proposal
or an investment, the aria-label of the marker isn't updated
automatically when we move the marker to a different place. This
behaviour will only affect people who use both a screen reader and a
mouse, since keyboard users can't change the position of the marker in
the first place. We'll deal with this issue when we make it possible to
change the position of a marker using the keyboard.
We forgot to do so in commit b896fc4bb. Back then, we said:
> Note that we aren't providing a proper aria-label for markers on the
> map we use in the form to create a proposal or an investment. Adding
> one isn't trivial given the current code, and keyboard users can't add
> a marker in the first place. We'll have to revisit this issue when we
> add keyboard support for this.
However, in the admin section, the marker is already there, so it should
have a label. In this case, we're using the coordinates as label because
it's the most relevant text for the marker in the context of a form. We
could also use "Default map location" instead, but that information is
already present on the page.
Axe was reporting the same accessibility error we mentioned in commit
b896fc4bb in this situation.
This way we remove duplication.
Note that to check whether to render the button to remove a marker,
we're checking whether the map location belongs to a mappable. This
means we're changing the code that renders the map in the "new proposal"
and "new investment" forms so the map location belongs to a proposal or
investment. We're association the map location to a new record because
writing something like:
```
def map_location
proposal.map_location || MapLocation.new(proposal: proposal)
end
```
Would change the `proposal` object because of the way Rails treats
non-persisted `has_one` associations. Although probably safe in this
case, changing an object when rendering a view could have side effects.
Also note that we're changing the HTML ID of the map element from
`admin-map` to `new_map_location` (the latter is returned by the
`dom_id` method). We were only using this ID in tests since commit
289426c1c, so changing it doesn't really affect us.
We were using the same texts twice. For the remove marker label text,
however, we were using the text defined in proposals for both proposals
and investments.
Ideally the translation keys for these texts would go in another
namespace, since they no longer refer to just proposals. However,
renaming the translation keys would mean losing the existing
translations in every language we manage through Crowdin. So we aren't
doing so.
People using these settings don't know about the hidden fields, but they
do know about the fields that are actually displayed on the page. So we
check that these fields are updated when the marker is updated.
We are unifying the test "Create budget - Knapsack voting (default)" with
"A new budget is always created in draft mode" because they are almost the same.
On the other hand, we also merged the test "update budget" with "submit the
form with errors and then without errors". Just like in the previous case, there
were two ways to access the edit page, so we removed the one that is already
tested in other specs.
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")
Axe was reporting an accessibility error:
```
Found 1 accessibility violation:
1) aria-command-name: ARIA commands must have an accessible name
(serious)
https://dequeuniversity.com/rules/axe/4.11/aria-command-name?application=axeAPI
The following 1 node violate this rule:
Selector: .leaflet-marker-icon
HTML: <div class="leaflet-marker-icon map-marker
leaflet-zoom-animated leaflet-interactive"
tabindex="0" role="button">
<div class="map-icon"></div>
</div>
Fix any of the following:
- Element does not have text that is visible to screen readers
- aria-label attribute does not exist or is empty
- aria-labelledby attribute does not exist, references elements
that do not exist or references elements that are empty
- Element has no title attribute
```
Using the title of the proposal/investment as the text of the marker is
definitely a good solution when there are several markers on the map.
Not sure whether there's a better option when there's only one marker,
though.
Note that we aren't providing a proper aria-label for markers on the map
we use in the form to create a proposal or an investment. Adding one
isn't trivial given the current code, and keyboard users can't add a
marker in the first place. We'll have to revisit this issue when we add
keyboard support for this.
We're also changing a test to make sure that titles with quotes in their
names don't break the markup due to an invalid aria-label attribute.
This way it's easier to know that the styles for the `break` HTML class
and the JavaScript for sortable elements (which we shouldn't use, by the
way, because of its accessibility issues) are only used here.
This method was returning a boolean value and caused a
`Naming/PredicateMethod` when upgrading rubocop.
So, instead, we're returning the created line when it was successfully
created, and `nil` when it wasn't.
Having said that, I'm not sure why we added the `.persisted?` back in
commit 3eb22ab7b since as far as I can tell we don't use the return
value for anything. The test added in commit da43e9e2e for this change
passes if we simply return `lines.create(investment: investment)`.
For now I'm leaving the `persisted?` check just in case, but removing it
might be fine.
This method was calling `check_availability`, which returned a boolean
value and caused a `Naming/PredicateMethod` when upgrading rubocop.
So we're changing the logic a little bit to remove the
`check_availability` method and merge the tests of `check_availability`
and `notifiable_available?` (which were almost identical) together.
We introduced this issue in commit f8faabf7d.
Since this component didn't have any tests (there are system tests for
it, though), we're also adding tests that check that only the right
buttons are rendered when accessing as administrator.
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
