So tests won't fail when an institution changes the default organization
name.
The tests are also easier to understand now, since it's more obvious
where the "CONSUL" text is coming from.
We were repeating the same code over and over (with a few variants) to
setup tests which require an administrator. We can use a tag and
simplify the code.
In some tables, we had "actions", and some columns were also links
pointing to some places. Having both of them at the same time is
confusing, particularly since traditionally the links in the columns
pointed to the same place as some of the actions (although that's not
the case since commit 48db31cd).
We're still keeping links in tables which don't have an action column.
For instance, the proposals table has a "select" button which would be
harder to use if we had action buttons next to it.
By using real XML responses developers will be able to understand better
how the integration works (the data flow), and the correspondency between
`remote_census` settings and their place at a real XML response.
As `stubbed_responses` methods were removed from the model layer now the
stubbing part should be managed from the test environment code so also
added a new helper module `RemoteCensusSetup` that can be used anywhere
where we need to call the web service.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Now we can use the checkbox label text directly as locator at spec,
also default web_sections are created before every spec by loading
`db/seeds.rb` file so we were duplicating "Proposals" WebSection.
We were allowing users to check/uncheck the "Visible to valuators"
checkbox even when the budget is finished and so the investments cannot
be edited. So users were still able to check/uncheck this attribute, but
the server was silently rejecting these changes.
We've considered removing the column in this case but decided to keep it
since users can already control which columns they'd like to display.
This rule was added in rubocop-rspec 1.39.0. The `visible: false` option
is equivalent to `visible: :all`, but we generally use it meaning
`visible: :hidden` for positive expectations and `visible: :all` for
negative expectations.
The only exceptations are tests where we count the number of map icons
present. I'm assuming in this case we care about the number of map icons
independently on whether they are currently shown in the map, so I'm
keeping the `visible: :all` behavior in this case.
By default, Capybara only finds visible elements, so adding the
`visible: true` option is usually redundant.
We were using it sometimes to make it an obvious contrast with another
test using `visible: false`. However, from the user's perspective, we
don't care whether the element has been removed from the DOM or has been
hidden, so we can just test that the visible selector can't be found.
Besides, using `visible: false` means the test will also pass if the
element is present and visible. However, we want the test to fail if the
element is visible. That's why a couple of JavaScript-dependant tests
were passing even when JavaScript was disabled.
These tests were supposed to check the link to vote is hidden when users
don't have permission to vote. However, they aren't testing that, since
the `visible: false` option also matches visible elements. The links are
actually considered visible since they're displayed by the browser;
there's just another element on top of them.
Using `obscured: true` instead of `visible: false` solves the issue.
However, while the `obscured` option is true when the element is hidden
by another element, it's also true when the element is not currently
visible in the browser window, so in some cases we need to scroll so the
condition is effective.
This rule was added in Rubocop 0.89.0. However, there are some false
positives when we don't use interpolation but simply concatenate in
order to avoid long lines. Even if there weren't false positives, there
are places where we concatenate to emphasize the point that we're adding
a certain character to a text.
We might reconsider this rule in the future, since we generally prefer
interpolation over concatenation.
Using `travel` we go to `Time.now + interval`. If the application's time
zone changes due to seasonal clock changes during that interval, we
might travel to a time which is not the time we intended to travel to.
Example:
On a system using the UTC time zone, it's 9AM on October 25 (Friday).
Since by default CONSUL uses the Madrid time zone, it means the
application's time is 11AM.
We use `travel` to advance three days. That means we go to 9AM UTC on
October 28 (Monday). The application's time will be 10AM due to the
seasonal clock change, so we haven't fully traveled three days in
application's time.
To reproduce locally, run:
```
TZ=UTC rspec spec/system/proposal_notifications_spec.rb:410
```
Using `travel_to` with `3.days.from_now`, which uses the application's
time zone and so it will travel to October 28 at 11AM, solves the
problem.
There were places where we had two links pointing to the same place; one
link would be the name/title of a record, and one link would be under
the "actions" column.
This is confusing, since users would probably expect these links to
point to different places (which is what happens in other tables in the
admin section) and might try to click one of them and then the other
one and be surprised when they found out both of them go to the same
page.
In commit 74083df1 we added the possibility to assign administrators and
valuators to budgets, so they would only manage the budgets they're
assigned to.
However, when filtering projects, we were still showing all
administrators and valuators as options to filter investments. It makes
more sense to only show the valuators and administrators assigned to the
current budget.
Note this change only affects the view, and so malicious users could
technically send any other administrator or valuator ID. In this case,
they would get empty results since those administrators/valuators
wouldn't have any investments assigned, so taking this case into account
is not necessary.
It was removed in commit 128a8164 because we hadn't reviewed it nor
tested it properly. We're now adding it again, fixing the issues we've
found while reviewing.
It worked differently after upgrading to jQuery 3. According to the
jQuery upgrade guide:
> It is almost always a mistake to use .removeAttr("checked") on a DOM
> element. The only time it might be useful is if the DOM is later going
> to be serialized back to an HTML string. In all other cases,
> .prop("checked", false) should be used instead.
We've had to add a couple of hacks in order to make jQuery UI datepicker
work with Turbolinks, and one of our tests is failing because the
datepicker changes its height when changing from a month with 5 weeks to
a month with 6 weeks.
We could add a workaround so the test still passes (jQuery UI doesn't
provide a configuration option to always displays 6 weeks in the
datepicker), but I think it's easier to just use the HTML5 native date
input field, which also allows us to simplify the code a bit and IMHO it
improves the user experience, particularly when using mobile phones.
Since date fields are not supported in Safari and Internet Explorer,
we're still using the jQuery UI datepicker on those browsers (and on any
other browser not supporting date fields).
Due to these changes, we're moving the tests checking datepicker's
behaviour to the dashboard. I've choosing not to change the public pages
because I'm not 100% sure everybody would like this change (some people
prefer the datepicker because we can configure the way it looks).
It's known that Foundation Sticky causes some renderization problems
when rendering sticky elements in anchored position.
The problem seems to be that Foundation Sticky is showing the
support box on medium and up devices overlapped with "Share" and
"Community" sidebar boxes when loading proposal page through
Turbolinks and when restoring the page from brwoser history.
Foundation seems to be doing some top property dynamic calculation
(javascript) and is setting top property to `206px` when it should be
`0px`. Notice that this do not happen on page first load (without
Turbolinks). Check foundation/foundation-sites issue 11098.
Another workaround could be to remove sticky feature for bigger that
small devices (medium large xlarge xxlarge).
Check foundation/foundation-sites issue 9892.
The JavaScript involved wasn't working since we removed the disable-date
attribute in commit 73ff6881.
We're also improving the JavaScript in two ways:
First, we trigger the `change` event immediately, so when the page loads
date fields are disabled when phases are disabled.
And second, we don't remove the selected dates when disabling a phase,
so disabling it and enabling it again will keep the selected values.
Rails automatically disables buttons when submitting a form. This works
fine most of the time: for AJAX requests, it enables them again after
the request is complete, and for non-AJAX requests, the button is
replaced by a new element when the new page loads.
However, there's an exception. When a request returns data so users can
download a fire, the request is not an AJAX one and the button is not
replaced. So users are left with a disabled button they can no longer
click.
So in this case, we aren't disabling the button after a user clicks it.
After updating foundation-rails in commit 58071fd6, the orbit slider
stopped working properly. That's because the `.orbit-slide` elements now
use a `position: absolute` rule, and so our rule for `.orbit-container`
elements making their height 100% (which we added in order to be able to
add images with different heights) makes them have a height of 0px,
since now the `.orbit-slide` elements are not part of the document flow
anymore.
Making the `.orbit-slide` elements have relative position fixes this
issue, but introduces a different one, producing a really bad-looking
animation when changing a slide.
So we're disabling the animation as well in order to avoid this jump.
This change also fixes another issue introduced in commit 58071fd6 which
caused slide controls to stop working when changing slides back and
forth.
It was being duplicated when restoring a page by using browser
history. With this solution we will avoid to have screen readers
descriptions more than once inside any sociual share button.
We need to use page body event delegation so it will work with any
element even with the ones added through ajax, in this case the
annotation comments box form. By doing this way we do not need
this code on the server response anymore.
Furthermore JS events defined at ajax responses are not part of
application javascript and are lost when restoring a page from
browser cache, you can try to apply the same event delegation
technique to the `erb` file and it wont work just because events
added dinamically are not treated the same than `application.js`
code.
To reproduce the error:
1. Load an annotatable draft version
2. Move to any other page
3. Go back
Now "Publish comment" button wont work.
If we do not destroy annotator app before storing the page at
browser cache we will unnecesarily initialize annotations twice (or
more) duplicating Annotator HTML markup and causing
unexpected errors.
Without this commit you will find an error when restoring a page with
annotator, you can click on any annotation and you will see the annotation
comments are being loaded twice.
IMO this is an idempotency issue within Annotator JS library.
Patch extracted from here the comments on turbolinks issue 253 and
converted to vanilla javascript.
The hide action over datepickers ensures us that opened datepickers
will be closed before leving the page. Previously if you open any
datepicker and then move to previous page you will keep seeing the
datepicker in the restored page.
Sometimes we define URLs for POST requests which are not defined for GET
requests, such as "/residence", so redirecting to it after signing out
results in a routing error.
So instead of using the request referer, we're using the stored location
devise uses, and we're not storing locations in POST requests.
For reasons I'm not sure about, the homepage (and the welcome pages)
were an exception in our "redirect users to the same page they were"
policy.
I'm not sure about the welcome pages (no test was present indicating
they should behave in a special way), but in the case of the home page,
it was a bit annoying to be redirected to a different place after
signing in.
When signing in from a page containing GET params, like
`/budgets/1/investments?heading_id=4`, we were redirected to a URL
without those GET params; in this case, `/budgets/1/investments`.
Using the request fullpath, as recommended in the devise documentation,
keeps these parameters when redirecting.
