It was removed in commit 128a8164 because we hadn't reviewed it nor
tested it properly. We're now adding it again, fixing the issues we've
found while reviewing.
We need to use page body event delegation so it will work with any
element even with the ones added through ajax, in this case the
annotation comments box form. By doing this way we do not need
this code on the server response anymore.
Furthermore JS events defined at ajax responses are not part of
application javascript and are lost when restoring a page from
browser cache, you can try to apply the same event delegation
technique to the `erb` file and it wont work just because events
added dinamically are not treated the same than `application.js`
code.
To reproduce the error:
1. Load an annotatable draft version
2. Move to any other page
3. Go back
Now "Publish comment" button wont work.
Note we're using a new sanitizer. Ideally we'd reuse the
`AdminWYSIWYGSanitizer`, but then code that would be correctly shown by
markdown-it (like the <h1> tag) wouldn't be shown on the web, which is
confusing. Ideally we would configure markdown-it to only allow the tags
present in the `AdminWYSIWYGSanitizer` and provide some kind of help
showing which tags are allowed.
The main obstacle to extract this partial was probably the paths for the
flag and unflag actions.
Now that we use Rails 5.1 `resolve` method to handle nested resources,
we can use `polymorphic_path`.
Also note the code is a bit ugly because comments render a divider. We
should probably use a CSS border instead.
Co-Authored-By: taitus <sebastia.roig@gmail.com>
We were treating legislation proposals as if they were proposals,
omitting the "legislation" namespace, and so we were flagging/unflagging
proposals when we wanted to flag/unflag a legislation proposal.
We were using inline styles and passing local variables around, while
the rule we were following is very simple: it's only hidden if it's a
form to reply to a comment.
We were using a <ul> tag for a single comment, where the first element
of the list was the comment itself and the second element was the list
of replies.
IMHO it makes more sense to have a list of all comments, where every
element is a comment and inside it there's a list of replies.
We're also rendering the list even if it has no children so it's easier
to add comments through JavaScript. Then we use the :empty CSS selector
to hide the list if it's empty. However, since ERB adds whitespace if we
structure our code the usual way and current browsers don't recognize
elements with whitespace as empty, we have to use the `tag` helper so no
whitespace is added.
This feature wasn't properly tested nor reviewed, and after reviewing
several pull requests with a similar status and considering this pull
request is related to the public area of the web, we've decided to
remove it before releasing version 1.1.
This commit reverts commit 4f50e67a.
The new CSV report was more configurable and could work on proposals,
processes and comments. However, it had several issues.
In the public area, by default it generated a blank file.
In the admin section, the report was hard to configure and it generated
a file with less quality than the old system.
So until we improve this system, we're bringing back the old investment
CSV exporter.
This commit reverts most of commit 9d1ca3bf.
We're going to change CKEditor to an inline editor, and the "ckeditor"
gem doesn't provide an option to do so.
Since using `cktext_area` would automatically generate a "classic"
iframe CKEditor, we need to use `text_area` and load the editor using
JavaScript. Personally I prefer this option anyway.
Note in the jQuery selector we need to use `textarea.html-area`; using
just `.html-area` would fail if there's an error message associated to
the textarea, since Rails will add the `.html-area` class to the error
message.
We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
We were converting markdown to HTML every time we saved a record, which
has the same problems as sanitizing HTML before saving it to the
database, particularly because the body of a legislation draft is stored
in a translations table.
Performance-wise this isn't a problem: converting a text with more than
200_000 characters takes about a milisecond on my machine.
Note we need to modify a migration generated by globalize, since the
method `create_translation_table!` would fail now that we don't define
`translates :body_html` in the model.
Sanitizing descriptions before saving a record has a few drawbacks:
1. It makes the application rely on data being safe in the database. If
somehow dangerous data enters the database, the application will be
vulnerable to XSS attacks
2. It makes the code complicated
3. It isn't backwards compatible; if we decide to disallow a certain
HTML tag in the future, we'd need to sanitize existing data.
On the other hand, sanitizing the data in the view means we don't need
to triple-check dangerous HTML has already been stripped when we see the
method `auto_link_already_sanitized_html`, since now every time we use
it we sanitize the text in the same line we call this method.
We could also sanitize the data twice, both when saving to the database
and when displaying values in the view. However, doing so wouldn't make
the application safer, since we sanitize text introduced through
textarea fields but we don't sanitize text introduced through input
fields.
Finally, we could also overwrite the `description` method so it
sanitizes the text. But we're already introducing Globalize which
overwrites that method, and overwriting it again is a bit too confusing
in my humble opinion. It can also lead to hard-to-debug behaviour.
In general, we always use relative URLs (using `_path`), but sometimes
we were accidentally using absolute URLs (using `_url`). It's been
reported i might cause some isuses if accepting both HTTP and HTTPS
connections, although we've never seen the case.
In any case, this change makes the code more consistent and makes the
generated HTML cleaner.
Sometimes we're interpolating a link inside a translation, and marking
the whole translations as HTML safe.
However, some translations added by admins to the database or through
crowdin are not entirely under our control.
Although AFAIK crowdin checks for potential cross-site scripting
attacks, it's a good practice to sanitize parts of a string potentially
out of our control before marking the string as HTML safe.
The name `safe_html_with_links` was confusing and could make you think
it takes care of making the HTML safe. So I've renamed it in a way that
makes it a bit more intuitive that it expects its input to be already
sanitized.
I've changed `text_with_links` as well so now the two method names
complement each other.
There's a case where we would face a Cross-Site Scripting attack. An
attacker could use the browser's developer tools to add (on their
browser) a `<code>` tag with a `<script>` tag inside in the text of the
draft version. After doing so, commenting on that text would result in
the attacker's JavaScript being executed.
This way we can simplify the way we generate form fields. In some cases,
we also use the human attribute in table headers, which IMHO makes
sense.
I haven't moved all of them: for example, sometimes a label is
different depending on whether it's shown to administrators, valuators,
or users. And I haven't touched the ones related to devise, since I
wasn't sure about possible side effects.
Note I've also removed placeholders when they had the same text as their
labels, since they weren't helpful. On the contrary, the added redundant
text to the form, potentially distracting users.
I'm not sure why it isn't already done by foundation's form builder. It
doesn't make any sense to change an ID of a form field without changing
the `for` attribute of its label.
Using the block syntax to generate the label with a <span> tag inside
isn't necessary after upgrading foundation_rails_helpers. Before the
upgrade, we couldn't do so because the <span> tag was escaped.
Moderate legislation proposals
- added a controller for moderation/legislation
- updated view to appropriate link + added route
- added a spec
- Feature test
- test for faded
- javascripts for visual effects
