While this bug was already present in the general admin search, the
combination of both search and filters was very uncommon. I've only
found this combinations in the users section, where you've got the
"erased" filter, but in this case searching for erased users doesn't
really make sense since their username and email have been deleted and
so there's nothing to find.
So the hidden content seemed to be the only affected section. However,
we're adding the field to every section so we don't have to make sure we
add it when we need it (like we did in the SDGManagement section).
We introduced this bug in commit 55d339572, since we didn't take hidden
records into consideration.
I've tried to use `update_column` to simplify the code, but got a syntax
error `unnamed portal parameter` and didn't find how to fix it.
We were ignoring the draft version param when loading an annotation,
which could result in a strange situation where we load an annotation
and a draft version different than the one it belongs to.
Thanks to this change, we can simplify the code a little bit. IMHO the
`comments` and `new_comment` routes should have been added on member
instead of on collection, which would further simplify the code. I'm
leaving the routes untouched just in case changing the URL has side
effects on existing installations.
We forgot to change the line rendering the image in commits 3574bf867c
and 810bdae37a, and so the custom image was being ignored.
Note that, in the test, we're stubbing a constant instead of adding a
new image. The main reason is that, if we add a new image, forks would
have to change the image when they change the `VALID_IMAGES` constant;
otherwise the tests would fail.
Rubocop was complaining about a Layout/ExtraSpacing in a couple of
places.
These issues weren't detected by Pronto because they didn't affect lines
changed in the pull request. These lines were fine until we removed the
lines next to them in commits 4b42a68b6 and 00f0c4410.
This test has been failing many times because it checks the database
after starting the process running the browser.
We're disabling JavaScript like we do in every other test using the
`create_proposal_notification` method. This way, Capybara will use the
rack driver and there'll be no risk of errors that currently might take
place if both the process running the browser and the process running the
test access the database.
When we perform database queries in tests after the process running the
browser has started, we sometimes get failures in our test suite due to
both the tests and the browser accessing the database at the same time.
Furthermore, using `Poll.all` results in a database query, and doing so
after the process running the browser has started might result in
failures when running our test suite.
We were clicking on the "Sign in" link right after clicking on the "Sign
out" link, which might result in simultaneous requests and exceptions
when running our test suite.
So we're adding an expectation to make sure the first request has
finished before starting the following one.
We were doing a `mappable.map_location` call in an `expect` which might
result in a database queries. Doing database queries in a test after the
process running the browser has started might result in exceptions while
running our test suite.
There were cases where we clicked the button to submit the form and
immediately we visited a different page.
In the past, we've had similar code produce PG::ProtocolViolation errors
in similar situations. Since we've had these errors a few times in the
nested imageable specs, there's a chance they're related to the absence
of the expectation.
Although I'm not even remotely sure this will fix these issues, at least
we now follow the convention of making expectations after every request.
Note we're changing both the nested imageable and nested documentable
specs. Only the nested imageable would need to be changed because it's
the one where there's a `visit` inside the
`imageable_redirected_to_resource_show_or_navigate_to` method. I'm
changing both for consistency.
We were missing a notice in this case. Not only this caused
inconsistencies in the user experience, but it also made it hard to add
an expectation in the test checking the request had finished before
making a new one. Simultaneous requests sometimes cause failures in our
test suite.
The idea to show the status of the existing features was done in commit
7339a98b74. Back then, we didn't have the separate `process.` prefix,
and so processes were enabled/disabled using settings like
`feature.debates` instead of `process.debates`.
IMHO making the information about the enabled features public could
potentially be a bit risky since it gives too much information about the
current status of the application.
Showing which processes are enabled, on the other hand, is pretty
harmless, and it's the reason why this feature was added in the first
place.
Currently with both seeds and dev_seeds, not only was this email not
displayed from the system emails section, but it also caused an error in
the application.
@email_to had an empty value and in the view we tried to access
@email_to.name which caused the error. We kept the same logic but
added the current_user to make sure it always has a valid value. We add
the current_user because the current_user is always present in this controller..
"Participate in the final voting" was a concept which was hard to
understand since many people would think it was related to the
voting/polls section and that somehow there was going to be a "final"
poll.
So we use "Vote for budget projects" instead.
Thanks Pomerange for the suggestion.
We had the same texts four times, with slight variations in the case of
the management section.
We're unifying them under the "verification" i18n namespace, since the
texts are about actions which can be done depending on whether users are
verified or not.
Note the names of the i18n keys aren't very consistent, since we use
"debates" in plural but "proposal" in singular. We're leaving it like
this so existing translations aren't affected.
We used "retire" because we translated it literally from the Spanish
verb "retirar" which can mean both "retire" and "withdraw".
Note we're still using "retire" in database fields and method names;
changing that might make it harder to upgrade from a previous version of
CONSUL.
The current consul GraphQL API has two problems.
1) It uses some unnecessary complicated magic to automatically create
the GraphQL types and querys using an `api.yml` file. This approach
is over-engineered, complex and has no benefits. It's just harder to
understand the code for people which are not familiar with the
project (like me, lol).
2) It uses a deprecated DSL [1] that is soon going to be removed from
`graphql-ruby` completely. We are already seeing deprecation warning
because of this (see References).
There was one problem. I wanted to create the API so that it is fully
backwards compatible with the old one, BUT the old one uses field names
which are directly derived from the ruby code, which results in
snake_case field names - not the GraphQL way. When I'm using the
graphql-ruby Class-based syntax, it automatically creates the fields in
camelCase, which breaks backwards-compatibility.
So I've added deprecated snake_case field names to keep it
backwards-compatible.
[1] https://graphql-ruby.org/schema/class_based_api.html
