Some tasks don't have to run on every tenant. The task to calculate the
TSV is only done for records which were present before we added the TSV
column, and that isn't going to happen in any tenants because we added
the TSV column before adding the tenants table. Similarly, the migration
needed for existing polls isn't necessary because there weren't any
tenants before we allowed to set the starting/ending time to polls.
We aren't adding any tests for these tasks because tests for rake tasks
are slow and tests creating tenants are also slow, making the
combination of the two even slower, particularly if we add tests for
every single task we're changing. We're adding tests for the
`.run_on_each` method instead.
The subdomain elevator we were using, which is included in apartment,
didn't work on hosts already including a subdomain (like
demo.consul.dev, for instance). In those cases, we would manually add
the subdomain to the list of excluded subdomains. Since these subdomains
will be different for different CONSUL installations, it meant each
installation had to customize the code. Furthermore, existing
installations using subdomains would stop working.
So we're using a custom method to find the current tenant, based on the
host defined in `default_url_options`.
In order to avoid any side-effects on single-tenant applications, we're
adding a new configuration option to enable multitenancy
We're enabling two ways to handle this configuration option:
a) Change the application_custom.rb file, which is under version control
b) Change the secrets.yml file, which is not under version control
This way people prefering to handle configuration options through
version control can do so, while people who prefer handling
configuration options through te secrets.yml file can do so as well.
We're also disabling the super-annoying warnings mentioning there are no
tenants which we got every time we run migrations on single-tenant
applications. These messages will only be enabled when the multitenancy
feature is enabled too. For this reason, we're also disabling the
multitenancy feature in the development environment by default.
Note we're using the `:HOST` regular expression since subdomains can
contain the same characters as domains do. This isn't 100% precise,
though, since subdomains have a maximum length of 63 characters, but is
good enough for our purposes.
This way all tenants will be able to access them instead of just the
default one.
The apartment gem recommends using a rake task instead of a migration,
but that's a solution which is primarily meant for new installations.
Migrations are easier to execute on existing installations.
However, since this migration doesn't affect the `schema.rb` file, we
still need to make sure the shared schema is created in tasks which do
not execute migrations, like `db:schema:load` or `db:test:prepare`, just
like the apartment gem recommends. That's why we're enhancing these
tasks so they execute this migration.
Note that there might be cases where the database user isn't a superuser
(as it's usually the case on production environments), meaning commands
to create, alter or drop extensions will fail. There's also the case
where users don't have permissions to create schemas, which is needed in
order to create the shared extensions schema and the schemas used by the
tenants. For these reasons, we're minimizing the number of commands, and
so we only alter or create extensions when it is really necessary.
When users don't have permission, we aren't running the commands but
showing a warning with the steps needed to run the migration manually.
This is only necessary on installations which are going to use
multitenancy; single-tenant applications upgrading don't need to run
this migration, and that's why we aren't raising exceptions when we
can't run it.
For new installations, we'll change the CONSUL installer so extensions
are automatically created in the shared schema.
Also note the plpgsql extension is not handled here. This is a special
extension which must be installed on the pg_catalog schema, which is
always in the search path and so is shared by all tenants.
Finally, we also need to change the `database.yml` file in order to
search for shared extensions while running migrations or model tests,
since none of our enabled extensions are executed during migrations;
we're also adding a rake task for existing installations. Quoting the
apartment documentation:
> your database.yml file must mimic what you've set for your default and
> persistent schemas in Apartment. When you run migrations with Rails,
> it won't know about the extensions schema because Apartment isn't
> injected into the default connection, it's done on a per-request
> basis.
The scripts crashed when the `data` folder wasn't present, which is the
common situation in development environments or production environments
not using Capistrano, since this folder isn't under version control.
In case we receive consecutive requests we are locking the poll author record
until the first request transaction ends, so the author answers count during
subsequent requests validations is up to date.
Note that the `create` action doesn't create an image but updates an
answer instead. We're removing the references to `:create` in the
abilities since it isn't used.
In the future we might change the form to add an image to an answer
because it's been broken for ages since it shows all the attached
images.
Adding, modifiying, and/or deleting questions for an already started
poll is far away from being democratic and can lead to unwanted side
effects like missing votes in the results or stats.
So, from now on, only modifiying questions will be possible only if
the poll has not started yet.
We need to update a couple of tests because a poll is created in the
tests with a timestamp that includes nanoseconds and in the form to edit
the time of the poll the nanoseconds are not sent, meaning it was
detected as a change.
We were already saving it as a time, but we didn't offer an interface to
select the time due to lack of decent browser support for this field
back when this feature was added.
However, nowadays all major browsers support this field type and, at the
time of writing, at least 86.5% of the browsers support it [1]. This
percentage could be much higher, since support in 11.25% of the browsers
is unknown.
Note we still need to support the case where this field isn't supported,
and so we offer a fallback and on the server side we don't assume we're
always getting a time. We're doing a strange hack so we set the field
type to text before changing its value; otherwise old Firefox browsers
crashed.
Also note that, until now, we were storing end dates in the database as
a date with 00:00 as its time, but we were considering the poll to be
open until 23:59 that day. So, in order to keep backwards compatibility,
we're adding a task to update the dates of existing polls so we get the
same behavior we had until now.
This also means budget polls are now created so they end at the
beginning of the day when the balloting phase ends. This is consistent
with the dates we display in the budget phases table.
Finally, there's one test where we're using `beginning_of_minute` when
creating a poll. That's because Chrome provides an interface to enter a
time in a `%H:%M` format when the "seconds" value of the provided time
is zero. However, when the "seconds" value isn't zero, Chrome provides
an interface to enter a time in a `%H:%M:%S` format. Since Capybara
doesn't enter the seconds when using `fill_in` with a time, the test
failed when Capybara tried to enter a time in the `%H:%M` format when
Chrome expected a time in the `%H:%M:%S` format.
To solve this last point, an alternative would be to manually provide
the format when using `fill_in` so it includes the seconds.
[1] https://caniuse.com/mdn-html_elements_input_type_datetime-local
We were testing that the `calculate_winners` method does not take the
price into account; we can do the same in a model test. We already have
a different system test to check that the price isn't displayed in the
view.
This method was introduced in Rails 6.0. It can be used to take an array
and create a hash where the elements of the array are the indexes of the
hash.
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.
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.
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.
After a user assigned as a budget admin deletes their account or gets blocked by
a moderator, the application throws an exception while loading the admin
investment index page.
As an erased user is not really deleted and neither its associated roles, the
application was failing when trying to sort and administration without a
username. In this case, the application was throwing an `ArgumentError:
comparison of NilClass with String failed` exception.
As a blocked user is not deleted or its roles, the application failed when trying
to access the user name through the delegation in the Administrator. In this
case, the application was throwing a `NoMethodError: undefined method `name' for
nil:NilClass` exception.
Add missing relation between administrator and budget_administrators, otherwise
we'd get the following exception when deleting and administrator with assigned budgets:
PG::ForeignKeyViolation:
ERROR: update or delete on table "administrators" violates foreign key constraint "fk_rails_ee7dc33688" on table "budget_administrators"
DETAIL: Key (id)=(3) is still referenced from table "budget_administrators".
I'd say this feature is actually tested in the "proposal polls specific
validations"; the empty test was probably added by accident in commit
4b8cc85c4.
This way the tests won't appear as "pending" when running the test
suite, and so we get rid of a lot of noise in the test results. There
doesn't seem to be a way to call `skip` without the test being marked as
"pending".
Note that in the globalizable tests we need to build a factory before
deciding whether an atribute is required or not (particularly for the
milestone factory, since milestone attributes are required depending on
the presence of other attributes). This isn't possible before we're
inside the test, so we can't add an `if:` condition to the test. So
we're adding the condition inside the test instead. A minor
inconvenience of this method is the test still runs even when the
condition is `false`.
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.
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.
