* update to node version 16
* Remove canvas compiled node objects
* use f35 testing image as builder/base
* Update production builder base image to use Fedora 35
I missed this (it's not immediately obvious that there are 2 base
images declared in this file, one called builder, one called
production, this seems like it could be refactored to lift that
out (give it a name, a cmd, and do nothing else) `as base` perhaps,
while keeping separate install processes for testing- and pr-/production images.
* Add libpq dependency in production
We need this (if not the -devel header file, at least the library) to
start pg_ext.so
I think this might have been working because of the --cache-from
options when building in the build container script?
* Update .gitpod.dockerfile
* Add temporary cleanup for upgrade to bin/setup
This has the undesirable effect of requiring a yarn reinstall.
It would be better if there were a smart check to confirm the version
of the canvas.node file matched the node version or did not (so we
only do this as needed, rather than on every setup invocation until
this is removed from the code).
* Use check-files rather than force when rebuilding canvas
Optimal situation would be a `rebuild` command in yarn (I believe npm
has this option) to recompile canvas (all that's needed) rather than
fetch, install, and build.
Co-authored-by: Michael Kohl <me@citizen428.net>
* test: whitespace unicode characters cannot be used as titles or tags
* feat: add localized error message
* refactor: use localized error message
* fix: whitespace unicode characters cannot be used as titles or tags
* chore: fix locale after merge
* refactor: fix indentation
* Fix spelling of prohibited in method names
Additionally, rubocop removed a redundant user_id validation since
Article belongs to user.
* Fix spelling
Missed one method call on the same line (fixed the first of two
instances needing changes)
* add failing test cases
The reorganization to remove let was due to a limit on nesting rspec
contexts (it inside context inside describe inside describe when
trying to use different titles in let blocks in contexts)
The first test was clarified (the "U+202D" string that looks like the
code for a unicode point is valid, but the character \u202d is
expected to be invalid
The remaining two tests are based on the feedback I'd given in the PR,
failing because we don't assert title is present after removing
unicode whitespace, and we don't actually set the title (gsub is
non-destructive, returning a value).
* Set title to sanitized title
contains_prohibitied_unicode_characters? was using == (which is not a
good match for strings to regexes), use =~ instead
Change invalid example characters in titles from \u202d (bidi override) to
\u200a (hair space)
Assert that the empty title is blank and can't be blank (even though
it contains no prohibitd characters after replacement)
* change the definition of prohibited characters
From the description, it looks like "unicode space characters" was the
desired rejection set. It was unclear what the existing regex was
matching on (I couldn't get the expected examples to match correctly).
Given the intent, I select "unicode space property, except the ascii
space character", which may _also_ be incorrect but passed the tests.
* Remove now-invalid expectation for presence of user id
Since this was redundant (belongs_to user), we no longer will validate
presence of user id, and we should not test for it.
* Only reject bidirectional text controls
The original issue pointed to the BIDI controls as problematic.
While they called out other "whitespace" characters, as we accept a
wider range of input languages, the need to handle non-printing
punctuation (for example a group separating space for some asian
languages).
It's possible a wider list of characters should be added - if that's
the case I suggest this regex and the sanitization be moved to a
standalone class, and each of the recommendations in
https://www.w3.org/TR/unicode-xml/#Charlist be checked for
applicability.
* Check for blank title after sanitizing disallowed characters
Since validate_title modifies the title, and doesn't set error
messages on the model itself, check for non-empty title _after_
potentially removing any disallowed characters.
* Remove invalid characters before validation
Remove the validate_title and contains_prohibited_characters methods,
and center all logic on the remove prohibitied unicode characters
method.
Remove unneeded and unused arguments (input string is always title,
replacement is always removal/empty string).
* handle case when validating null title
If the title's nil, we don't want to call match? since it will fail.
Title will sometimes be nil when validating.
Co-authored-by: Ben Halpern <bendhalpern@gmail.com>
Co-authored-by: Dan Uber <dan@forem.com>
* Refactoring and documenting class
I came to this commit by looking for raw `where` method calls to the
Article object. Originally, I was looking at `where.not(user_id: :id)`
but found this service class.
In this class, I saw some significant duplication and went with a bit of
refactoring and some documentation.
This refactoring removed some unnecessary guards and calculations (as
documented inline).
* Naming a constant and adding more documentation
* Renaming/rearranging constants for clarity
As I'm writing about the Unified Embed project and creating
documentation for the upcoming Forem Fest, I realized that the naming
convention created confusion.
This commit is an effort to tidy up that confusion.
* Normalizing implementation pattern
* Use the most recent timestamp from sent digest messages
The original logic here had queried for up to 10 (unordered) messages
from the database for this user, perhaps leveraging the knowledge that
we purge old messages after 90 days to ensure 10 was enough.
Since the last message in the limited and unordered list could have
had any sent_at time in the past 90 days, it's possible users could
pass the "should send email" check multiple times in a day (and get
multiple digest emails, I'm not certain when the rake task runs but it
could be as frequently as once per deployment?)
Since we're only using this list of messages to find the most recently
sent message, select the maximum sent time.
* convert periodic_email_digest setting to days
Time.current - last_email_sent_at gives an integer count of elapsed
seconds
Settings::General.periodic_email_digest is an integer count of days
between digests.
Convert to days (so we're only sending digests when enough time has
elapsed) instead of defaulting to once every 2 seconds (or at least
several times per day).
* Clean up comparison
Rather than subtracting the current time from the last time, and
checking that the difference in time is greater than the periodic
setting, use days.ago to get the timestamp far enough in the past, and
ensure last email was sent before that ( `sent_at < n.days.ago` ).
This seems like it reads clearer than the original implementation.
* Prefer Time.before? to numeric comparison
Change the method name from last_email_sent_at to last_email_sent, so
that the comparison reads like English (the other callers use it as a
number)
And since the code reveals its intention clearly, there's no need for
an inline comment about the next line any more.
* Refactoring to consolidate logic
Prior to this commit, two controllers had nearly identical chunks of
logic. This refactor extracts the logic to a common and more canonical
location.
* Addressing rubocop's aggressive auto-fix
* Adding spat operator for pluck
* Renaming method for greater clarity
* Load article before creating a page view for an invalid one
This should resolve a validation error in PageView.create! when the
article does not exist (perhaps it was deleted, or the user suspended,
or it was invalid data sent from the client).
This had been happening dozens of times per day for the last 10
months.
* Use an intention revealing symbol instead of calculated id
Since we only require that find_by not find anything, pass a symbol
that's not going to be the id for any article under any conditions.
As an added benefit, this provides a clear indication of the purpose
of the symbol, without needing to mentally confirm that
```sql
SELECT * FROM articles
WHERE id IN (
SELECT (1 + MAX(id)) FROM articles
) LIMIT 1
```
actually never gives any articles back.
* Refactoring to leverage an Article scope
I've been looking at the queries that involve filtering articles based
on scores. There's several places that seem to be close to consistent,
but have some nuanced difference.
This refactor consolidates one of those "almost the same" cases.
* Bump for travis
* Add missing spaces
* Use filter_map over map + reject/compact
* Simplify FactoryBot calls
* Use to_h with block instead of map + to_h
* Use guard clause
We are seeing test failures when an id like "aaabbbcccdd1m" match with
"1m" as the time parameter. I think we only want to match the time
when a ?t= or ?start= (and the permissive &t= or &start=, which might
only be part of the larger REGISTRY_REGEXP and either not effective or
not needed for the video id pattern)
The goal here is these should be valid
"aaabbbcccdd"
"aaabbbcccdd?t=1"
"aaabbbcccdd?start=1h23m55s"
"aaabbbcccdd&t=1"
but not these
"aaabbbcccddt=1"
"aaabbbcccddstart=1"
"aaabbbcccdd123456h"
The same logic may be appropriate to backfill into the prior regexp as
well, my immediate concern is with randomly generated 12-15 character
strings from Fake getting matched during testing (where they were
expected to raise an error during id parsing).
* Remove slash characters from user supplied user search input
Prevents an error when the search term includes '\'
PG::SyntaxError: ERROR: syntax error in tsquery
https://app.honeybadger.io/projects/66984/faults/79391397
I had originally thought to add this cleanup to Search::Username but
decided to move it as close to the generated (invalid) query as
possible to prevent alternate paths finding their way here.
* Add spec
Since there's no existing tests for the scope - I put the test code on
the caller (Search::Username) rather than the model (User), this seems reasonable.
* When the term is empty (or only slashes) just return null relation
* Handle nil input (search for nothing) correctly
One of the request specs sends a username search with no query, so we
can't call nil.delete or nil.empty?, use blank? of empty?
The 400 and 403 error pages show json parsing errors in most browsers,
since "Error: Bad Request" is not a valid json body ('"Error: Bad
Request"' would be, or the object with key error and value of the
string which I've selected is _also_ valid).
Do we have frontend code that's looking for this body before it parses
for some reason, or was this just a mistaken copying from the api
controller in the initial PRs (#2293 for not_authorized, and #6248 for
the bad_request method, which may have just replicated the decision
for not_authorized)?
* Favoring single SQL and computation over enum
Prior to this commit, we ran a SQL statement to generate a list of
articles then applied some minor randomization.
With this commit, I'm removing the randomization in favor of expected
values, and collapsing the result set into a single inline SQL and some
post query simple arithmatic.
Let's check my math. For each article we sum:
* `article.score`
* `article.comments_count` * 14
* a random number between 0 and 5 (`rand(6)`) with expected value of 2.5
* the tag's (taggings_count + 1) / 2
The new SQL query sums the `article.score` and the
`article.comments_count` * 14. I then reduce the remainder of the
equation. From "for each article sum `(rand(5) + (taggings_count + 1) /
2)`" we have the following:
`article_count * (2.5 + (taggings_count + 1) / 2)`
Which is equivalent to:
`article_count * ((taggings_count + 1 + (2.5 * 2)) / 2)`
Which is equivalent to:
`article_count * ((taggings_count + 6) / 2)`
This change reduces computation time by favoring expected values.
* Fixing broken behavior and adding comments
With this refactor, I'm adding documentation and favoring using a common
method that wasn't available at the time of implementation.
In [this commit][1] we had logic that said "if we have a singular tag
use the cache" otherwise use the join. At that time, the implementation
of [Article.cached_tag_with][previous] was as follows, allowing only a
singular tag:
```ruby
scope :cached_tagged_with, ->(tag) { where("cached_tag_list ~* ?",
"^#{tag},| #{tag},|, #{tag}$|^#{tag}$") }
```
The [current implementation][current], as of writing this, allows for
multiple tags and is as follows:
```ruby
scope :cached_tagged_with, lambda { |tag|
case tag
when String, Symbol
# In Postgres regexes, the [[:<:]] and [[:>:]] are equivalent to "start of
# word" and "end of word", respectively. They're similar to `\b` in Perl-
# compatible regexes (PCRE), but that matches at either end of a word.
# They're more comparable to how vim's `\<` and `\>` work.
where("cached_tag_list ~ ?", "[[:<:]]#{tag}[[:>:]]")
when Array
tag.reduce(self) { |acc, elem| acc.cached_tagged_with(elem) }
when Tag
cached_tagged_with(tag.name)
else
raise TypeError, "Cannot search tags for: #{tag.inspect}"
end
}
```
Given that we are content to use the cached tag in the singular tag
case, it seems safe to say that we're comfortable using it in the
multiple tag case.
[1]:af5a391429 (diff-24503fd25ed68e6ebceee4951bc4f9b255b197278d4aa9d86ef9d5afe3f26bea)
[previous]:af5a391429/app/models/article.rb (L151)
[current]:98e97e7aa8/app/models/article.rb (L212-L227)
* Refactoring questions asked of user
In this pull request, I'm extracting and normalizing role-based
questions asked of the user.
Prior to this commit, our codebase has asked two very similar questions
of our user model:
- `user.has_role?(:admin)`
- `user.admin?`
In asking `has_role?(:admin)` we are relying on implementation details
of the rolify gem. In addition, the `has_role?` question asked
throughout controllers or views means that it's harder to create
hieararchies of permissions.
In favoring `user.admin?` as our question, we can use that indirection
as an opportunity to discuss and decide "Should someone with the
`:super_admin` role be `user.admin? == true`?"
The details of this commit is to do three primary things:
1. Ask the `has_role?` questions in "one place" in the code (e.g. the
`Authorizer` module)
2. Extract the role based questions that are on the `User` model and
provde backwards compatable delegation.
3. Structure the code so that it's harder to accidentally call
`user.has_role?` (e.g., make `User#has_role?` and `User#has_any_role?`
private).
This is related to #15624 and the updates are informed by discussion in
PR #15691. This commit supplants #15691.
* Refactoring the liquid tag policy tests
* Fixing typo
* Bump for travis
