This feature was only enabled for proposals five years ago, and it
hasn't changed since then. The pending test only gets in the way.
Implement. Or implement not. There is no pending.
They were marked as pending.
Note Capybara doesn't support finding a button by its `aria-labelledby`
attribute, so we're using the ugly `click_button "Yes"`, like we did in
commit fabe97e50.
It looks like it was disabled because it was failing sometimes for some
reason. I haven't found the reason, though; we're changing the test a
little bit to make it easier to read. Enabling it will let us find out
whether it still fails.
This file only has tests related to tags; if the model doesn't have
tags, we simply wouldn't include `it_behaves_as` in their tests instead
of including it and then skipping it.
The map feature was never implemented for debates (only for proposals
and budget investments) and it was crashing for debates because the page
didn't load the geozones. And we don't have a "geozone" field in the
debates form either.
So we're removing the map page alongside its (pending implementation)
tests.
CONSUL doesn't implement blank votes via web; the comment was based on
the code used in Madrid, which was actually very complex.
And the concept of "all city" was also specific to Madrid. Poll
questions aren't associated to a geozone, so the geozone will depend on
the poll they're associated to.
We are use a display: block style for labels containing check boxes inside
them, and the label has a width of 100%.
This means that clicking on the blank space on the right of the label text
will check/uncheck the checkbox. To avoid this behaviour we modify the
"display" attribute of the labels.
In order to prevent unexpected behaviour in terms_of_service form labels,
we add specific css for this case when define a checkbox within the
.actions class.
Avoid displaying the price in admin budget headings section
and avoid fill the field 'price' in admin budget headings form
when the budget has been checked with hide_money field.
The application crashed when we generated hints to attributes with
interpolation arguments in their `human_attribute_name`.
When generating the hint, we used the `custom_label` method to generate
a label and get the `for` attribute and, since we weren't passing a
text, it used the default human attribute name for the field. However,
it crashes if the default attribute name requires an interpolation
argument.
So now, since we were only using the `custom_label` method in order to
get the `for` attribute, we're simply passing an arbitrary text to the
method.
One of these tests was failing sometimes on Github Actions. It looked
like the line `custom_banner.save!` was using the validations from the
Banner class sometimes, even if the callbacks had correctly been
removed in the DummyBanner class.
Se we're inheriting from ApplicationRecord instead of inheriting from
Banner. Since I couldn't reproduce the issue locally after running the
test hundreds of times and with the same seed and tests that were
running on Github Actions, there's a change this won't work. I've tested
a few times on Github Actions and it seems to be working, but we'll have
to keep an eye on it.
There are CONSUL installations where the validations CONSUL offers by
default don't make sense because they're using a different business
logic. Removing these validations in a custom model was hard, and that's
why in many cases modifying the original CONSUL models was an easier
solution.
Since modifying the original CONSUL models makes the code harder to
maintain, we're now providing a way to easily skip validations in a
custom model. For example, in order to skip the price presence
validation in the Budget::Heading model, we could write a model in
`app/models/custom/budget/heading.rb`:
```
require_dependency Rails.root.join("app", "models", "budget", "heading").to_s
class Budget::Heading
skip_validation :price, :presence
end
```
In order to skip validation on translatable attributes (defined with
`validates_translation`), we have to use the
`skip_translation_validation` method; for example, to skip the proposal
title presence validation:
```
require_dependency Rails.root.join("app", "models", "proposal").to_s
class Proposal
skip_translation_validation :title, :presence
end
```
Co-Authored-By: taitus <sebastia.roig@gmail.com>
In SQL, conditions like:
```
tag_id IN (x) AND taggable_type='Debate' OR taggable_type='Proposal'
```
Don't work as intended; we need to write:
```
tag_id IN (x) AND (taggable_type='Debate' OR taggable_type='Proposal')
```
Due to this bug, we were returning taggings for proposals without
intending to do so.
Since the code was very hard to read, we're also simplifying it.
We were getting a warning in Rails 6:
DEPRECATION WARNING: ActionView::Base instances should be constructed
with a lookup context, assignments, and a controller.
We were using this hack in order to allow `File.new` attachments in
tests files. However, we can use the `fixture_file_upload` helper
instead.
Just like it happened with `file_fixture`, this helper method doesn't
work in fixtures, so in this case we're using `Rack::Test::UploadedFile`
instead.
This way we don't have to write `"spec/fixtures/files"` every time.
Note this method isn't included in factories. We could include it like
so:
```
FactoryBot::SyntaxRunner.class_eval do
include ActiveSupport::Testing::FileFixtures
self.file_fixture_path = RSpec.configuration.file_fixture_path
end
```
However, I'm not sure about the possible side effects, and since we only
use attachments in a few factories, there isn't much gain in applying
the monkey-patch.
We were using custom rules because of some issues with Paperclip. These
rules work fine, but since we're already using the file_validators gem,
we might as well simplify the code a little bit.
Just like we did with regular attachments, we're moving the logic to
generate URLs out of the model.
Note we're changing the `image_path_for` helper method in order to
return a `polymorphic_path` because sometimes it's used in combination
with `favicon_link_tag`, and `favicon_link_tag` doesn't automatically
generate a polymorphic URL when given an `ActiveStorage::Attachment`
record.
This fixes a few issues we've had for years.
First, when attaching an image and then sending a form with validation
errors, the image preview would not be rendered when the form was
displayed once again. Now it's rendered as expected.
Second, when attaching an image, removing it, and attaching a new
one, browsers were displaying the image preview of the first one. That's
because Paperclip generated the same URL from both files (as they both
had the same hash data and prefix). Browsers usually cache images and
render the cached image when getting the same URL.
Since now we're storing each image in a different Blob, the images have
different URLs and so the preview of the second one is correctly
displayed.
Finally, when users downloaded a document, they were getting files with
a very long hexadecimal hash as filename. Now they get the original
filename.
This way we fix a bug we mentioned in commit 930bb753c which caused
links to documents to be broken when editing their title because the
title was used to generate the URL of the document.
Note we're still using Paperclip to render cached attachments because
this is the only case where we store files with just Paperclip and not
Active Storage.
With Active Storage, we render attachments just like any other resource,
using `polymorphic_path`. Paperclip included the `url` method in the
model; since the model doesn't have access to the request parameters
(like the host), this was inconvenient because it wasn't possible to
generate absolute URLs with Paperclip.
In order to simplify the code and make it similar to the way we used
Paperclip, we're adding a `variant` method accepting the name of a
variant and returning the variant.
