Before the "valuating" phase, all investments have undecided feasibility
and none have been selected, so the filters would return no results
(except the "not_unfeasible" one, which would return everything).
We removed it in commit c322b2c4a because it was hard to know the
difference between "Feasible" and "Not unfeasible". We're renaming the
"Not unfeasible" filter instead.
We're also moving the "selected" filter so it appears before the
"unselected" filter, just like the "feasible" filter appears before the
"unfeasible" filter.
Many CONSUL installations struggle when trying to define their own
settings or trying to overwrite the default ones. Particularly because
tests assume the default settings are used, so changing them breaks the
test suite in many cases.
So we're adding a custom setting model in order to make it easier to
change the default settings and define new ones, while making sure doing
so won't break the tests.
While these settings can also be changed by administrators, defining
custom settings in the code is useful in come cases. For example, when
certain default settings have been decided while using staging
environments and there's interest in using the same settings on
production, when institutions would like to use the same custom settings
across several CONSUL installations, or (when we implement multitenancy)
when defining the same default settings for each new tenant.
Calculating winners before the balloting is over is useless (results
aren't published at that point) and can lead to the wrong results since
users are still voting and results might change.
And we were showing the button to calculate winners even when a budget
had finished. However, in this case the action to calculate winners did
nothing, which resulted in administrators seeing nothing happened after
pressing the button.
IMHO selecting in how many headings it's possible to support investments
isn't necessary when there's only one option to choose from. It's
obvious that if there's only one heading, it will be impossible to
select investments from more than one heading.
When we try to register with omniauth and the email or username already exists,
we use the finish_signup and do_finish_signup actions to allow the user to choose
another email or username.
The do_finish_signup action of the registration controller calls the
send_oauth_confirmation_instructions method which is responsible for sending the
confirmation email.
In this method we were only validating the case that the email is duplicated. Now
we add one more condition that allows us to send the instructions for the case in
which we have had to change our username.
We weren't showing the details of answers without a description, even if
they had images, videos or documents. Some users found that behavior
unexpected since the description isn't a mandatory field and so they
left it blank, but they added images to that answer and they didn't
appear on the poll page.
Note we had a condition not to show the title of an answer when it had
no description. I think that condition was redundant because answers
without a description weren't loaded in the first place. Anyway, that
condition doesn't make sense anymore because we're displaying answers
with images but no description.
Adding the error to the translation means Rails will automatically show
the error message in the view.
We're also adding a test to make sure the error message is correctly
displayed.
We now have duplication in the validation rules, however. Validating
translatable attributes is still a bit of mess.
Since targets didn't have a title but only a long description, every
form allowing to select targets was pretty much unusable: we either
displayed just the code or the whole description.
Now, with a concise title, it's easier to find and select the desired
target.
The titles have been copied from The Global Goals page [1].
Note we're using the `short_title` I18n key for the `title` method and
the `long_title` I18n key for the `long_title` method. We can't use
`title` as I18n key instead of `short_title` because it would affect
existing translations.
[1] https://www.globalgoals.org/
We didn't enable it by default in commit 676adfcb3 so existing
installations didn't suddenly get a new section without expecting it.
But since the setting already exists for installations using version
CONSUL 1.3, now it will only be enabled for new installations.
We were having issues with cached attachments and external services.
A `Tempfile` is returned by `URI.open` when using S3, so we're dealing
with this case as well.
In order to migrate existing files from Paperclip to ActiveStorage, we
need Paperclip to find out the files associated to existing database
records. So we can't simply replace Paperclip with ActiveStorage.
That's why it's usually recommended [1] to first run the migration and
then replace Paperclip with ActiveStorage using two consecutive
deployments.
However, in our case we can't rely on two consecutive deployments
because we have to make an easy process so existing CONSUL installations
don't run into any issues. We can't just release version 1.4.0 and 1.5.0
and day and ask everyone to upgrade twice on the same day.
Instead, we're following a different plan:
* We're going to provide a Rake task (which will require Paperclip) to
migrate existing files
* We still use Paperclip to generate link and image tags
* New files are handled using both Paperclip and ActiveStorage; that
way, when we make the switch, we won't have to migrate them, and in
the meantime they'll be accessible thanks to Paperclip
* After we make the switch, we'll update the `name` column in the active
storage attachments tables in order to remove the `storage_` prefix
Regarding our handling of new files, the exception are cached
attachments. Since those attachments are temporary files used while
submitting a form and we have to delete them afterwards, we're only
handling them with Paperclip. We'll handle these ones in version 1.5.0.
Note the task creating the dev seeds was failing after these changes
with an `ActiveStorage::IntegrityError` exception because we were
opening some files without closing them. If the same file was attached
twice, it failed the second time.
We're solving it by closing the files with `File.open` and a block. Even
though we didn't get any errors, we're doing the same thing in the
`Attachable` concern because it's a good practice to close files after
we're done with them.
Also note we have to change the CKEditor Active Storage code so it's
compatible with Paperclip. In this case, I haven't been able to write a
test to confirm the attachment exists; I was getting the same
`ActiveStorage::IntegrityError` mentioned above.
Finally, we're updating the site customization image controller to use
`update` so the image and the attachment are updated within the same
transaction. This is also what we do in most controllers.
[1] https://www.youtube.com/watch?v=tZ_WNUytO9o
This way screen reader users will know which record they're going to
access when focusing on a link to a certain action. Otherwise they'd
hear something like "Edit, link", and they wouldn't know which record
they'll end up editing if they follow the link.
User testing has shown this filter isn't really useful and sometimes
makes users wonder what it's about. This is particularly true in CONSUL
installations which don't change the default values (most of them),
since users will see a filter with options like "Official position 1".
We were using helper methods inside the model; we might as well include
them in the model and use them from anywhere else.
Note we're using a different logic for images and documents methods.
That's because for images the logic was defined in the helper methods,
but for documents the logic is defined in the Documentable concern. In
the past, different documentable classes allowed different content
types, while imageable classes have always allowed the same content
types.
I'm not sure which method is better; for now, I'm leaving it the way it
was (except for the fact that we're removing the helper methods).
The same way it's done for images.
We were converting the number of megabytes to bytes and then converting
it to megabytes again. Instead, we can leave it as it is and only
convert it to bytes when necessary (only one place).
The chances of an unpublished proposal appearing on the homepage was
very low because only the proposals with the most votes appear there and
unpublished proposals don't have any votes. However, it was technically
possible on new sites where only a few proposals had been created.
Users were unable to reset a translation to its original value after
updating it because we weren't storing anything in the database in that
case.
I've considered deleting the existing translation when this happens. I'm
not sure about which approach is the better one, so I'm using the less
destructive one.
We're not adding the rule because it would apply the current line length
rule of 110 characters per line. We still haven't decided whether we'll
keep that rule or make lines shorter so they're easier to read,
particularly when vertically splitting the editor window.
So, for now, I'm applying the rule to lines which are about 90
characters long.
Our previous system to delete cached attachments didn't work for
documents because the `custom_hash_data` is different for files created
from a file and files created from cached attachments.
When creating a document attachment, the name of the file is taken into
account to calculate the hash. Let's say the original file name is
"logo.pdf", and the generated hash is "123456". The cached attachment
will be "123456.pdf", so the generated hash using the cached attachment
will be something different, like "28af3". So the file that will be
removed will be "28af3.pdf", and not "123456.pdf", which will still be
present.
Furthermore, there are times where users choose a file and then they
close the browser or go to a different page. In those cases, we weren't
deleting the cached attachments either.
So we're adding a rake task to delete these files once a day. This way
we can simplify the logic we were using to destroy cached attachments.
Note there's related a bug in documents: when editing a record (for
example, a proposal), if the title of the document changes, its hash
changes, and so it will be impossible to generate a link to that
document. Changing the way this hash is generated is not an option
because it would break links to existing files. We'll try to fix it when
moving to Active Storage.
Instead of adding the padding to each individual element inside the
container, why not adding padding to the container itself? The answer is
"because we want the background of the children elements to take the
width of the whole screen". But this generates either HTML cluttered
with elements to add padding or repetitive padding definitions in the
CSS.
So now we only define the padding once, and when an element requires a
full width background or border, we use the `full-width-background`
mixin.
In this case the code is a bit more complex because the header is also
used in the dashboard and admin layouts:
* In the public layout, the body has a margin, so we include the mixin
to take margin into account
* In the dashboard layout, the header itself has a margin, so we include
the same mixin
* In the admin layout, the headet doesn't have a margin but gets the
whole width, so in this case we include the mixin which dosen't take
the margin into account
In the future, the idea is to apply this principle to the <body>
element and remove the `@include grid-column-gutter` in the CSS as well
as the `small-12 column` classes in the HTML.
Note we use the `calc()` function inside the mixin instead of using it
in the `$full-width-margin` variable. That way we avoid nested `calc()`
operations, which don't work in Internet Explorer.
Also note we're using `flex-grow: 1` to make one element appear on the
left of the screen and the other one on the right. It would be easier to
use `justify-content: space-between` (which is actually the default for
the top-bar element). However, there's a bug in Internet Explorer and
old versions of Firefox; they include the absolutely-positioned
`::before` element we use to set the full width background when
calculating where to position the elements. The bug was fixed in Firefox
52 (released in 2017).
Finally, we're removing the padding from our logo. In order to allow
logos like the new one and at the same time provide backwards
compatibility to logos in existing CONSUL installations, we're relaxing
the validation rule for the logo width.
