System tests are used to test the application from the user's point of
view. To test for specific exceptions, particularly regarding
authorization permissions, controller tests fit better.
Another option would be to test the page displayed shows a certain text,
like "Internal server error". I'm choosing controller tests because
they're faster and we're basically testing the same scenario many times
and we've already got a test checking what happens when users access a
page raising an exception.
Tests are easier to read now. Besides, since we changed the inputs in
the admin section so they don't use jQuery but an HTML date field,
formatting with %d/%m/%Y might not work depending on the browser's
locale.
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.
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).
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.
This is the reason why this feature was implemented in the first
place: it's easy to open the editor, make some changes, close it, and
continue without realizing the changes have not been saved.
In the rest of the forms, this functionality is quite lacking. For
starters, some forms warn if there are unsaved changes, while some forms
don't, which is highly inconsistent and disorients users.
Furthermore, we were having problems with this feature after upgrading
Turbolinks, particularly in forms using CKEditor. In these cases, a lot
of hacking needs to be done in order to make this feature work properly,
since CKEditor adds some formatting automatically, and if this is done
after the form is serialized, we'll get some unexpected behavior. On the
other hand, comparing the value of a textarea against its `defaultValue`
property will work on every edge case, including using the browser's
back button or reloading the page.
Finally, users are used to the way web forms work, and aren't used to be
asked for confirmation when they change their mind and decide to leave
the page without saving the changes. Asking them for confirmation will
be annoying in most cases. Besides that, if they accidentally leave the
page, they can use the browser's back button and they'll recover the
unsaved changes.
It's true this won't happen it they accidentally close the browser's
window, but our WatchFormChanges functionality didn't work in this case
either. Using the "beforeunload" event adds more problems than it
solves, since it doesn't support custom messages (or, to be more
precise, modern browsers ignore custom messages), and it doesn't get
along with turbolinks.
Co-Authored-By: Senén Rodero Rodríguez <senenrodero@gmail.com>
