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
After commit 0214184b2, this method was only used in two places and was
only useful in one of them. IMHO it isn't worth it add a monkey-patch
for such a minor usage.
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>
We were getting a warning in Rails 6.0:
DEPRECATION WARNING: Class level methods will no longer inherit scoping
from `public_for_api` in Rails 6.1. To continue using the scoped
relation, pass it into the block directly. To instead access the full
set of models, as Rails 6.1 will, use
`ActsAsTaggableOn::Tag.default_scoped`.
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 already support Errbit and Airbrake as error monitoring services.
Since some people might not want to setup Errbit and might prefer
Rollbar over Airbrake, we're referencing it in the custom gemfile.
The integration between Airbrake/Errbit and DelayedJob wasn't working
because we require our dependencies in alphabetic order, and so by the
time `airbrake` was required, `delayed_job` wasn't loaded.
So we're manually requiring the integration.
SVG files are smaller than PNG files, can be compressed, and are
scalable.
We're choosing to render SVG files as images instead of inline because
inline SVGs aren't cached nor compressed, and they might appear several
times on the same page, making the generated HTML much larger.
We could also load them with an SVG sprite, using `<use>`, which would
reduce the number of HTTP requests when several icons are present on the
page (like in the index of most sections). However, using SVG inside an
`<img>` tag is universally supported, while the `<use>` tag doesn't
work in Internet Explorer when using an external URI and support in
Opera Mini and UC Browser is unknown.
We're already using a custom controller to handle direct uploads.
Besides, as mentioned by one of Active Storage co-authors [1], the
Active Storage DirectUploadsController doesn't provide any
authentication or validation at all, meaning anyone could create blobs
in our database by posting to `/rails/active_storage/direct_uploads`.
The response there could be then used to upload any file (again, without
validation) to `/rails/active_storage/disk/`.
For now, we're monkey-patching the controllers in order to send
unauthorized responses, since we aren't using these routes. If we ever
enable direct uploads with Active Storage, we'll have to add some sort
of authentication.
Similar upload solutions like CKEditor don't have this issue since their
controllers inherit from `ApplicationController` (which includes
authorization rules), while Active Storage controllers inherit from
`ActionController::Base`.
[1] https://discuss.rubyonrails.org/t/activestorage-direct-uploads-safe-by-default-how-to-make-it-safe/74863/2
We were getting hundreds of "warning: URI.escape is obsolete" messages.
So we're using `URI::DEFAULT_PARSER.escape` instead.
IMHO it's OK to add this monkey-patch because we're replacing Paperclip
with Active Storage, and when we finish with that we'll delete this
file.
This way, when the language is written form right-to-left, elements
using Foundation mixins/classes will float to the opposite direction as
they do in left-to-right languages. The same will apply to text
alignment.
To offer full support for RTL languages, we need to change every single
reference to `float: left`, `float: right`, `text-align: left`,
`text-align: right`, and possible adjust other properties like `left`,
`margin-left`, `padding-left` or `border-left`. In the meantime, we at
least partially support these languages.
Replacing `float` with `flex` when possible would also improve RTL
support.
The spinner was added in version 2.0.0, so by disabling it we're keeping
the behavior we've got in existing CONSUL installations.
The problem with the spinner is installations need to add a secret token
and share it across instances (when they've got more than one), and if
they don't, invisible captcha might report false positives.
I guess we could use `Rails.application.secrets.secret_key_base` as
secret token, but since we haven't tested this feature at all, it's
better to disable it for now.
Our `namespace` helper returns a string. However, Rails version 5.2.4.6
doesn't allow strings as arguments to polymorphic_path [1]
Since returning a symbol in our `namespace` helper would break other
places in the application, we're converting it to a symbol in the
methods calling `polymorphic_path`.
[1] https://github.com/advisories/GHSA-hjg4-8q5f-x6fm
Pronto is an external tool which is use to check code conventions and is
not needed to run the application, just like rubocop or scss-lint.
Loading it caused a couple of issues. First, it loaded BetterHtml, and
we had to disable it in order to avoid crashes in the development
environment.
Second, it loaded ruby-progressbar, which loads the ProgressBar class,
which conflicts with our own ProgressBar class. This made the
application crash whenever the ProgressBar class was used.
This way developers can run the checks on their machines and using
`bundle exec` we guarantee the right versions of all our gems are being
used; with Hound we can only use the versions supported by their
service.
When including the pronto-erb_lint gem, we're getting errors in
development where our ERB does not follow the conventions Better HTML
expects. Since we only use Better HTML because ERB Lint depends on it,
and right now we are not ready to follow its conventions, we're
disabling it.
Note pronto depends on rugged, which requires CMake and pkg-config to
build the `libgit2` library it depends on. CMake and pkg-config are
installed by default in some GNU/Linux distributions like Ubuntu, but
might not be installed on other systems, so we're adding them as
development dependencies.
We were using a custom icon because in the past social-share-button
didn't have support for whatsapp. But now that it does, we can remove
our custom icon.
Note we're using the `_app` suffix because that's the name of the icon
meant for mobile devices.
Note we're adding the icons in English on both the `sdg/en/` folder and
the `sdg/default/` folder. The latter stores the default icons when the
desired language does not have a fallback with an icon folder.
Also note we need to explicitely add the images to the asset pipeline
because they've been added to the `vendor/` folder; for some reason,
everything works properly without adding them to the asset pipeline if
we use the `app/` folder instead.
We do a similar thing with the settings helper, caching settings on a
per-request basis.
Using an instance variable in a helper reduces the amount of times we
need to calculate the cache key during a single request.
Even if Rails caches SQL queries per request, the test suite is faster
with this change, and we get rid of many redundant queries in the logs.
This way we avoid fetching each translation every time it's requested.
This reduces the amount of queries in the development logs and also
makes the test suite faster.
Since data for this model (title and description) is not generated in
CONSUL but by the United Nations, we aren't storing it in the database
but in our YAML translation files.
The reasoning is as follows. Suppose that, a few months after CONSUL
gets SDG support, a new language is added to CONSUL.
With YAML files, getting the texts in the new language would mean
updating CONSUL to include the new language.
But if we store these texts in the database, it means we have to update
the databases of all existing CONSUL installations, either each
installation by themselves (duplicating efforts) or running a rake task
(which we would have to write each time).
So we believe using translations works better in this case.
We're still storing records in the database with the code, so they can
be easily referenced via `has_many` or `has_many :through` associations.
The original devise_security_extension gem has not been maintained for
years. Its last release was version 0.10.0, and wasn't compatible with
Rails 5, and so we were using its master branch.
Since the gem was unmaintained, it was forked as devise-security and the
aforementioned master branch was released as version 0.10.1. This
version wasn't published in Rubygems, though, so we're now using the
first version that was published in Rubygems and had a release
announment [1].
Dependabot will probably open a pull request to upgrade to the latest
version, but for now I'm trying to keep the devise-security gem as
similar as the version we were using to make sure they're compatible,
particularly considering we're monkey-patching some of the modules
provided by this gem.
[1] https://github.com/devise-security/devise-security/releases/tag/v0.11.1
While Rails provides a lot of functionality by default, there's one
missing piece which is present in frameworks like Django or Phoenix: the
so-called "view models", or "components".
It isn't easy to extract methods in a standard Rails view/partial, since
extracting them to a helper will make them available to all views, and
so two helper methods can't have the same name. It's also hard to
organize the code in modules, and due to that it's hard to figure out
where a certain helper method is supposed to be called from.
Furthermore, object-oriented techniques like inheritance can't be
applied, and so in CONSUL customizing views is harder that customizing
models.
Components fix all these issues, and work the way Ruby objects usually
do.
Components are also a pattern whose popularity has increased a lot in
the last few years, with JavaScript frameworks like React using them
heavily. While React's components aren't exactly the same as the
components we're going to use, the concept is really similar.
I've always liked the idea of components. However, there wasn't a stable
gem we could safely use. The most popular gem (cells) hasn't been
maintained for years, and we have to be very careful choosing which gems
CONSUL should depend on.
The view_component gem is maintained by GitHub, which is as a guarantee
of future maintenance as it can be (not counting the Rails core team),
and its usage started growing after RailsConf 2019. While that's
certainly not a huge amount of time, it's not that we're using an
experimental gem either.
There's currently a conflict between view_component and wicked_pdf.
We're adding a monkey-patch with the fix until it's merged in
wicked_pdf.
We can remove the `new_framework_defaults_5_2` file by using Rails 5.2
default options and overwriting the ones we haven't enabled.
We're disabling `use_authenticated_message_encryption` because, even if
we don't use it, some CONSUL installations might be using it, and
enabling this options would make it harder to decrypt existing encrypted
messages.
And we're disabling `cache_versioning` until we verify our cache keeps
working and expires as expected on production environments, particularly
for stats.
This is enabled by default in Rails 5.2 applications.
Note this change will cause all fragment caching to expire. We consider
it acceptable considering the page where caching is most important
(stats) is barely affected by this change, since this change only
affects the view, and the time-consuming operations are cached in the
model.
Comments are actually affected, though, and pages with thousands of
comments might take a few extra seconds to load the first time they're
accessed after this change. We don't think this is going to be an issue
on existing CONSUL installations.
This is the default encryption for cookies in Rails 5.2 applications.
The reason it isn't enabled automatically for existing applications is
these cookies are not compatible with running the application on several
servers when some of them use Rails 4. Since this isn't our case (we
don't support using different versions of CONSUL on different servers),
and existing cookies are still read correctly, we can safely enable it.
We were manually adding forgery protection to all our controllers, but
in Rails 5.2 there's an option (enabled by default for new applications)
which adds this protection to all controllers.
Even if we don't use form_with, it makes sense to configure it to behave
the same way form_for does.
This is the default option in Rails 5.2 applications. IMHO it should
have been the default option for Rails 5.1 too, since generally form
inputs need an ID so they can easily be associated with a label.
All the code in the `bin/` and the `config/` folder has been generated
running `rake app:update`, except the `escape_javascript_fix` file,
which we've removed since the code there is already included in Rails
5.2.
We were checking tho project_id but not the project_key. However, in our
example secrets file we actually include a project_id but not a
project_key, and so new CONSUL installations would crash in production
environments.
Although technically the application doesn't crash if no host is
defined, we're also ignoring the current environment if no host is
defined, since errbit/airbrake is unlikely to work in this scenario.
Since some people hosting errbit might be using it as an internal tool
with a self signed certificate, we need to patch Airbrake so it accepts
these certificates.
With Errbit, you can set up your own server and host the information
regarding your exceptions there. You can also hire Airbrake's hosting
services or easily setup Errbit on Heroku.
We're still including the rollbar gem so we don't harm CONSUL users who
are using rollbar.
Note Errbit requires an old version of Airbrake which forced users to
configure the gem. So we're adding the current environtment to
`ignore_environments` when the project id isn't defined; this way the
application won't crash in this case.
Using pg_search 2.0.1 with Rails 5.2 results in deprecation warnings:
DEPRECATION WARNING: Dangerous query method (method whose arguments used
as raw SQL) called with non-attribute argument(s):
"pg_search_978c2f8941354cf552831b.rank DESC, \"tags\".\"id\" ASC".
Non-attribute arguments will be disallowed in Rails 6.0. This method
should not be called with user-provided values, such as request
parameters or model attributes. Known-safe values can be passed by
wrapping them in Arel.sql().
We're not upgrading to the latest pg_search because it only supports
ActiveRecord >= 5.2.
Using `pluck("DISTINCT")` was raising a warning in Rails 5.2:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s): "DISTINCT
taggings.tag_id". Non-attribute arguments will be disallowed in Rails
6.0. This method should not be called with user-provided values, such as
request parameters or model attributes. Known-safe values can be passed
by wrapping them in Arel.sql().
Since there was only one other use of distinct, I've decided to change
both of them in the same commit, even if the second one wasn't raising a
warning.
These columns were causing Rails 5.2 to throw a warning when ordering by
them, as if they weren't valid column names:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s):
:"budget/investments_count". Non-attribute arguments will be disallowed
in Rails 6.0. This method should not be called with user-provided
values, such as request parameters or model attributes. Known-safe
values can be passed by wrapping them in Arel.sql().
This change also makes their names consistent with the rest of our
tables and columns.
