There are three major things occurring in this pull request:
1. Renaming `Article#update_cached_user` to `Article#set_cached_entities`.
2. Reducing an organization's direct knowledge of which of the org's
attributes an article caches.
3. Removing duplicate calls to update the article associated with the
organization.
For renaming to `Article#set_cached_entities`, the prior method implied
we were updating the persistence layer. However, we were not making any
save nor update calls. This rename should clarify intention.
For reducing knowledge, the comments for
`Article::ATTRIBUTES_CACHED_FOR_RELATED_ENTITY` should explain the details.
And last, removing the duplicate calls; we had three methods that were
attempting to build and update the `Article#cached_organization`'s
value.
Closesforem/forem#17041
* Normalize article titles
* Change method name to match what it does
* Expand on allowed emoji ranges
This required a few changes to be in compliance with Rubocop because
that regex is a beast.
In looking at forem/forem#16787, it felt like we would benefit from a
similar approach as we adopted for nullifying blank strings.
This refactor helps pave the way for a "normalize_text_for" method (or
some such thing; naming things is hard).
This change also involved introducing a `describe` block into the
associated spec.
* feat: remove the default email and cobine the periodic digest and the contact email under the Email section
* refactor: rename the email_link to contact link and use the contact_email as a default and fallback to the ForemInstance.email
* chore: alignment
* feat: use the contact_email helper
* feat: move the contact_email to the ForemInstance model
* feat: use ForemInstance.contact_email instead of the application helper method
* removed the application Helper
* feat: set the dafault on the contact_email
* fix: cypress tests
* Update app/lib/constants/settings/general.rb
Co-authored-by: Michael Kohl <me@citizen428.net>
Co-authored-by: Michael Kohl <me@citizen428.net>
* Add boolean attribute main_image_from_frontmatter to indicate if cover image was set via frontmatter
* use article model spec for main_image_from_frontmatter test cases
* Add data migration script and spec for main_image_from_frontmatter for articles
* Update app/models/article.rb
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Co-authored-by: Michael Kohl <me@citizen428.net>
In my quest to find `where(attribute: value)` in controllers and even
services, I stumbled upon this pattern.
As of this commit (and prior) an Article's associated user is it's author.
* 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 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
* Refactoring to use StringAttributeCleaner
In #15281, we introduced the StringAttributeCleaner. Let's use it!
* Adding StringAttributeCleaner for User properties
* Parameterizing callback for attribute cleaning
* Reading and applying the example for attribute cleaner
* Refactoring Spam Handler
There's considerable repeated logic between checking spam for an article
and spam for a comment and user.
This attempts to send things through channels that are similar and close
in organization.
* Fixing broken spec
* Fixing spec around recent user
* Update app/models/reaction.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Update app/models/reaction.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Consolidating new user query logic
Prior to this commit there were two separate queries around new user
logic. With this commit, we're changing the logic to repurpose a site
wide setting.
* Generalizing a previously specific message
* Fixing method name
As part of a recommended refactor, I extracted a method, then renamed
it. I failed to account for that renaming.
This commit fixes that.
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Favoring dependent: :delete_all over :destroy
The `dependent: :destroy` callback is slower than `dependent: :delete`
and `dependent: :delete_all`. We need only favor the `dependent:
:destroy` when there's callbacks that happen.
In the case of :destroy, ActiveRecord instantiates each object and then
runs destroy. Whereas in the case of :delete, ActiveRecord issues a SQL
command to delete the related files.
It is often "safer" to use :destroy, as it guarantees that you'll
instantiate the record and run it's callbacks. But sometimes you have
to go with the speed of SQL.
This relates to #14140. Note there is still more to consider, but given
that we're looking at moving from :destroy on all of an article's page
views to :delete, we might buy enough time in the callback.
* Removing redundancy of article destroy
Prior to this commit, `before_destroy_actions` called the `bust_cache`
method which in turn called `touch_actor_latest_article_updated_at` but
`bust_cache` did not pass the destroying parameter. Then
`before_destroy_actions` immediately called
`touch_actor_latest_article_updated_at` with `destroying: true`.
With this change, we remove one of those duplicate calls.
* Fixing specs regarding relationships
* Fixing specs regarding relationships
* Refactoring and extending article spam behavior
Prior to this refactor, we were checking an Article's title for
spaminess. This change now checks the Article's title and
body_markdown.
In addition, the encapsulates the regular expression implementations in
favor of asking the Settings::RateLimit class to determine if the passed
in text is "spammy".
Furthermore, instead of having lots of inline logic within the article,
this refactor introduces a `Spam::ArticleHandler` class. This helps
tidy up the already busy Article specs by delegating the business logic
of Spam handling to it's own class.
There are further refactors for Comments and Users that would follow
this pattern, but this change is more important to get out the door.
Closes#15396
* Adding comments to clarify behavior of spam handling
* Adding missing expectation to spec
This commit removes a handful of magic numbers and instead relies on a
constant.
This has a small impact in that the Basic feed will now return 50
articles instead of 25. However, normalizing the feed pagination window
size helps reduce some oddities in reporting.
In addition, this might be something we consider giving administrators
the ability to set (with default options, because we shouldn't allow
page sizes of 10_000 as that's a massive memory hog).
Related to #14709
* Caching sum privileged reaction scores
This relates to work on improving the feed. Namely that in the current
proposal in PR #15240 I'm not accounting for how privileged users have
given feedback on an article.
With this change, I will now have a cached value on the articles table
that can be a proxy for how the privileged users have reacted.
In addition there's a small refactor that moves a constant to the
correct scoping object.
Dependent on #15283.
* Fixing method name to `exists?`
Prior to this commit, I was using `exist?` which doesn't work for
ActiveRecord::Relation objects.
* Initial work to expand detection functionality
* Finish up functionality and tests
* Fix class name
* Fix tests
* Update spec/services/articles/enrich_image_attributes_spec.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Update spec/services/articles/enrich_image_attributes_spec.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Separate followed and "anti-followed" tags
* Minor code change
* Indicate "anti-followed" tags in sidebar
* Fix UpdatePointsWorker spec
* Cries in Arel
* Use antifollow scope more selectively
* Add spec for not_cached_tagged_with_any scope
* Add explanatory comment
* Remove unused route
* Update comment
* Remove unrelated change, figure out why > 1 later
* Need more ☕
* Avoid false positives in spec
* Change handling of ingored tags in sidebar
* Add explanatory comment
* Move logic to determine noindex from view to model
This brings attention to the code and allows for faster unit tests.
I expect we'll be modifying this soon to deemphasize the code tag as a
marker of quality.
* Replace "5" constant with home feed minimum score
* Remove check for "code" tags in low quality article check
This made sense for DEV and other devrel communities, but is no longer
necessary for Forems.
* Add UserExperience setting for minimum index score
- add setting, I used `index_minimum_score` to match the
`home_feed_minimum_score`, `minimum_index_score` feels natural but
breaks the pattern.
- use this to determine whether to include article in the sitemap
- use this to determine whether to add noindex meta to article show page
* Remove unneeded tests
Since there's no longer any check for <code> tags in the body, omit
those tests (which passed anyway).
* Add new setting to admin page
* fix: artiles from the same feed can be fetched from multiple accounts
* undo unintentional changes to schema.rb
* modify: article_spec and import_spec tests
* fix: failing test
* modify: import_spec:213 for user.setting
* fix: failing test import_spec:213
* add: unique index on articles canonical_url where published
* revert: schema.rb
* modify: unique url error message to include admin email
* modify: custom error message
* modify: .update to .update! (bang)
* Simplify logic around generated article description
@citizen428 helpfully pointed out ActiveSupport's String#truncate
method, and I had read the processed_description method body
immediately before and knew it could benefit from using truncate.
When the body text is present, if it's long, truncate to 100
characters (with trailing ellipsis as needed), removing newlines.
When the body text is absent, populate with a generic "post by user"
message.
* Change magic description length to match the original behavior
Since we were previously slicing [0..100] we would get 101 characters, and then
appending three dots makes for 104 total.
There is a longish description passed in the articles api request test
that shows "part of the body as a description" that failed to match
with the first pass.
* Create DiscussionLocks
* Fix specs
* Update nullify_blank_notes_and_reason
* Update before_validation call
* Updated DiscussionLockPolicy for clarity
* Move permitted_attributes to a constant
* Update route
* Apply suggestions from code review for frontend
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Add title tags
* Wrap unlock confirm in main element
* Wrap flash messages up in div
* Actually fix title tags
* Hide comment reply button when discussion is locked
* Add E2E tests
* Try to fix E2E tests
* Cypress...you work locally but not in CI...why!?
* PR feedback
* Update E2E tests
* More E2E updates 😭
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Add HairTrigger spec
* Add weights to columns tsvectors
* Add correct sorting for search results
* Remove Timecop.travel
* Changed order of columns in trigger based on weights
* Add inline animated image detection
* Do not detect animation during preview
* Promote FastImage as a dependency
* Add Articles::DetectAnimatedImages service
* Restore previous parsing
* Add and use Articles::DetectAnimatedImagesWorker
* Remove obsolete poc
* Properly detect images in the after commit callback
* Simplify logic
* Use a second guard clause
* Fix parsing of relative paths and add tests
* Have Articles::DetectAnimatedImages correctly detect relative images uploaded locally
* Change Articles::DetectAnimatedImagesWorker priority to medium
* Only one & required
* Rename SiteConfig
* More renaming
* Update spec
* Update mandatory settings mapping
* More renaming
* e2e test fixes
* You have a rename, and you have a rename
* Spec fix
* More changes
* Temporarily disable specs
* After-merge update
* Undo rename for migration
* undo rename of DUS
* Fix DUS
* Fix merge problem
* Remove redundant DUS
* Fix specs
* Remove unused code
* Change wrong class name
* More cleanup
* Re-add missing values to constant
* Fix constant
* Fix spec
* Remove obsolete fields
* Add accidentally removed field
* Update spec
* Move methods from Settings::General to ForemInstance
* Remove unneeded model
* Change mentions of 'site config'
* Initial work for @-mention notifications from posts
* Revert article.published changes to article updater, add clarifying comments
* Extract article preview into reusable partial for notification views
* Clean up Article Updater
* Address + remove some FIXMEs
* Add a whole buncha specs for @-mention functionality in posts YAY
* Refactor create all spec to use shared examples, add clarifying comments
* Add guard clause to create all service
* Update new mention and notifiable action specs
* Some additional cleanup
* Add specs + shared examples to SendEmailNotificationWorker spec
* Use aggregate_failures where applicable
Co-authored-by: Michael Kohl <citizen428@dev.to>
* Cleanup and address code review comments
* Add MentionDecorator + relevant specs
* Address comments/issues flagged by @rhymes
* Optimize plucking user_ids when checking for article followers
Co-authored-by: Michael Kohl <citizen428@dev.to>
* Update format specifiers in readable_publish_date
* Trigger Travis manually
* Update format specifiers in readable_publish_date
* Update format specifiers in readable_publish_date
Co-authored-by: rhymes <rhymes@hey.com>
* Rename comment-mentioned-user class added by Html::Parser
* Add DataUpdateScripts for resaving articles, comments, and messages
* Swap order of messages and articles DUS
