**tl;dr** This PR is looking to provide a means to programmatically
assemble, from system configurations, what is currently the
[Articles::Feeds::WeightedQueryStrategy::SCORING_METHOD_CONFIGURATIONS][1].
Once we merge this PR, instead of having that constant, we'll initialize
the Articles::Feeds::WeightedQueryStrategy with the variant
configuration that we've assembled.
**Introduction**
This pull request has quite a bit going on, but as of now the production
code does not use any of it.
*Note: None of this code is bleeding into production code paths.*
Put your Ruby hat on and let's go for a ride. And apologizes for not
making this a smaller pull request. As I built this, I made many small
commits, but this became the smallest commit that provided the most
context without integrating into production code.
Before we get started, you may want to familiarize yourself with where
we're going. The `./app/models/articles/feeds/README.md` provides
further guidance; but fair warning reader, I have not reconciled the
README's language with what emerged as I wrote this pull request.
Those of you who are part of Forem Core team, you can read the
[Refinements to Our Present Feed Configuration][2]
to provide some insight into what's happening.
**Why not reconcile?** *Because I want to have all of the language and
thoughts available for you to help consider how best to name and model
this.*
**On to the Review**
The purpose of this pull request is to provide a mechanism for
engineering to provide a series of variant query levers. And to allow
us to easily configure those available query levers into a variant
query.
*Why the mix of Ruby configuration and JSON?*
The Ruby levers are meant to indicate that this is code we don't want to
expose in text based configuration because it might create SQL inject
points. (More on that when we integrate the code of this PR into the
production implementation)
The JSON levers are meant to indicate that "anyone" can pick the
appropriate/available levers and configure how they are set. In other
words, these values do not create SQL injection issues.
**What I Need from You**
I am putting this forward as a draft so you can read this code ahead of
time. I'm then going to schedule a synchronous code review where we
record our collective walk through of the implementation.
Closes:
- forem/forem#17268
- forem/forem#17269
- forem/forem#17270
- forem/forem#17271
- forem/forem#17273
Relates to:
- forem/forem#17308
- forem/forem#17245
[1]:6818ef3ed0/app/services/articles/feeds/weighted_query_strategy.rb (L84-L231)
[2]:https://forem.team/jeremy/refinements-to-our-present-feed-configuration-1p0c
This commit modifies two things:
1. The sort order of the feed.
2. Locking in the next incumbent for the feed.
In consultation with Jamie regarding the SQL, I've switched from a WHERE
IN type clause to creating a JOIN. This allows for the calculated
relevancy score to become a sortable value for the feed.
Second the incumbent feed is a slight modification of the past winner;
based on the results of a long running experiment. This adjustment is
per conversations with product.
The past experiment ran since early February, so it's time to retire it,
regroup and move forward.
Closesforem/forem#17307
* ContextNotification model
* Create and destroy context notifications when notifications are sent/unsent
* Added a transaction when creating notifications and context notifications
* Documentation for context notifications
There are two problems engrained in this:
1. The undocumented expectation of `/new` needed to other sites linking
with prefill information.
2. The prefill parameters being lost during the authentication process.
The first bug was introduced as part forem/forem#16536. The second is
likely introduced as part of a Rails upgrade (namely `request.path` most
likely once included the query parameters but with our current
implementation it does not).
Closesforem/forem#17292
Prior to this commit, we were looking at trusted user's reaction
counts. This is not the desired column to be testing.
See
f1d6291d00/app/models/reaction.rb (L25-L28)
```ruby
counter_culture :reactable,
column_name: proc { |model|
PUBLIC_CATEGORIES.include?(model.category) ? "public_reactions_count" : "reactions_count"
}
```
In the above code, all public reactions (hearts, unicorns, reading
list) go to public_reactions_count. Other things go to the
`reactions_count`; which is the thumbs up and thumbs down.
Fixesforem/forem#17260
* Use sluggerize to produce a slug from podcast episode title
This is essentially the process we use for Article#title_to_slug
without the random trailing bits on the end (since podcast episodes
had not worked that way previously).
* Add test case for slug generation
* Oh, the test only works if you checkin the fixture, too
* Award badges for github commits with various milestones
* Fixes on award badges to github commits
* Add data update script to add badges
* Disable award_multi_commit_contributors
* Revert "Disable award_multi_commit_contributors"
This reverts commit 1852f34fd45bd23b716cf80ebbd9ce2c1879a819.
* Remove DUS
* Change badge name
* Only use award_contributors
* Simplify specs
* Revert "Simplify specs"
This reverts commit 230514ccea68445b057991d3d536fbb2a590a3b1.
* Revert "Only use award_contributors"
This reverts commit a92735a079f43610aedc805f74f032466d23f1cf.
Co-authored-by: Mac Siri <krairit.siri@gmail.com>
In helping track down forem/forem#17041 I was looking at our cache
busting logic. We were looking up a constant via the `.const_get` call
from a string already defined inline. This refactor short-circuits
naming a string, capitalizing it, then looking in the class's registered
constants.
There's also a subtle bug in the original; When `provider` equals
"another_cache", the `#capitalize` method would return
`"Another_cache"`, whereas `#classify` will return `"AnotherCache"`.
Below are some benchmarks for the change.
```ruby
require "benchmark"
module EdgeCache
class Bust
def by_class
Fastly
end
def by_const_get
self.class.const_get("fastly".classify)
end
end
end
Benchmark.bmbm do |x|
x.report("by_class") do
1000.times do
EdgeCache::Bust.new.by_class
end
end
x.report("by_const_get") do
1000.times do
EdgeCache::Bust.new.by_const_get
end
end
end
```
```shell
> bin/rails runner /Users/jfriesen/git/forem/bench.rb
Rehearsal ------------------------------------------------
by_class 0.001239 0.000186 0.001425 ( 0.001342)
by_const_get 0.011398 0.000136 0.011534 ( 0.011538)
--------------------------------------- total: 0.012959sec
user system total real
by_class 0.000973 0.000030 0.001003 ( 0.000969)
by_const_get 0.011102 0.000032 0.011134 ( 0.011104)
```
There are two existing listeners for the `Audit::Logger`: `:moderator`
and `:internal`. (Note: during tests we ignore the :moderator and
:internal logs as defined in [config/initializers/audit_events.rb][1].)
Using `rg "Audit::Logger\.log\(:internal," --files-with-matches`, the
`:internal` listener is found in:
- app/controllers/admin/secrets_controller.rb
- app/controllers/admin/settings/base_controller.rb
- app/controllers/admin/settings/general_settings_controller.rb
Using `rg "Audit::Logger\.log\(:moderator," --files-with-matches`, the
`:moderator` listener is used in:
- app/controllers/rating_votes_controller.rb
- app/controllers/comments_controller.rb
- app/controllers/stories/pinned_articles_controller.rb
- app/controllers/admin/response_templates_controller.rb
- app/controllers/admin/tags_controller.rb
- app/controllers/admin/articles_controller.rb
- app/controllers/admin/users_controller.rb
- app/controllers/admin/reactions_controller.rb
- app/controllers/admin/tags/moderators_controller.rb
- app/controllers/tag_adjustments_controller.rb
- app/controllers/reactions_controller.rb
The `admin/spaces#update` action is most similar to the `admin#settings`
actions, which is why I chose `:internal`. I am looking for further
guidance on documenting this little area of the application (in
particular providing a data dictionary of :internal and :moderator).
Closesforem/forem#16957
[1]:https://github.com/forem/forem/blob/main/config/initializers/audit_events.rb#L9-L11
follow on to #15942 which enabled downloading existing DEV feeds, this
allows setting a feed url to DEV.
It's possible this user agent should be brought inline with the
tags choice of community name and url, but in the short term I'm
making this consistent with the existing feed importer.
* prefer case to multiple if branches
case klass works fine on inheritance chains (we don't need to match on
inheritance explicitly).
* Verify case on error class behaves as before
* Fix tests
I don't know how I committed tests that were failing, but don't do that
* Don't suggest users for an anonymous visitor to follow
The sidebar raises an error when rendering suggested users for a tag
when the user is signed out.
https://app.honeybadger.io/projects/66984/faults/79342962/
* Assert empty response returned for anonymous requests
* Only fetch RSS of users who can create articles
This commit leverages changes from forem/forem#16732 and
forem/forem#16763 to ensure that we're only fetching RSS feeds for
folks who have permission to create articles.
In addition, I chose to rename the variable `user_scope` to
`users_scope` to bring consistency in the variable names.
I also chose to refactor a private method so that it wasn't setting two
instance variables but was instead returning a value that could be used
to set an instance variable.
Closesforem/forem#16487
* Amending method name
This change makes it easier to resolveforem/forem#16487.
What do I mean by that? To address forem/forem#16487, I need to only
select user's who are authorized to create articles. In
forem/forem#16732 I added a method that will allow chaining of a User
scope. So with this current commit and forem/forem#16732, I'm
triangulating on an approach that will make that change easier.
I did not want to conflate those two, as mixing this PR's change and
what is necessary for the closing PR would create a more complicated
review. Not unduly complicated, but one that will require more tests
and a change in logic. And for someone reviewing the diff, those
concerns could easily be lost.
Yes, I have changed the method signature, but that method signature is
limited to one location:
```shell
❯ rg "Feeds::Import.call"
app/workers/feeds/import_articles_worker.rb
21: ::Feeds::Import.call(users_scope: users_scope, earlier_than: earlier_than)
```
So I look to the method signature a bit as an internal API, hence the change.
Related to forem/forem#16487
* don't set nil when size not called
recently, #16570 worked around an issue where aggregated siblings
wasn't present, by only calling size if present.
Unfortunately, this causes a comparison of integer (new json_data
aggregated siblings count) with nil (result of the safe navigation for dig()&.size
assigned nil to previous_siblings_size, when we expected it to be
0). The initial error on the same data moved farther down the controller.
If old_json_data is present, but does not have an array in
reaction.aggregated_siblings, we want to have previous size be zero.
Remove guard clause and previous assignment
The issue was not that old_json_data was nil (it came from an existing
notification) but that there were missing keys (or rather that the
json data for some notifications didn't have a reaction key at all).
Remove the guard clause and either set to the size, or zero if there
is none. I don't understand what the guard clause was for, so replace the
safety by adding a nil safe call to dig. I don't _believe_ this is
possible but in case it was we can shorten the check here.
* Add test to cover missing json_data['reaction'] key
* Remove unneeded temporary variable
* feat: allow reply_to and email_from to be set for an SMTP config
* feat: use these SMTP values in the mailers
* spec: test the application_mailer
* fix: validate with email, and not url
* feat: mimic macs changes from https://github.com/forem/forem/pull/16216 to use in this PR
* setup packs for admin
* refactor: order the keys and add a const for the auth methods
* feat: rename the header to a more user friendlly name
* chore: move the section with Emails
* feat: add a toggle that will show and hide the SMTP form under certain conditions
* feat: add the javaScript to handle the toggles
* feat: add a better description until we convert to a dropdown
* feat: ensure that we have declared sendgrid_enabled
* chore: add anote to the config controller
* chore: remove references of the email addresses to keep brnach scoped
* feat: tweak js
* test: cypress workflow to update smtp settings
* feat : update the smtp tests
* remove comments
* update test
* chore: rename NOTE
* feat: polisha dn test ForemInstance.only_sendgrid_enabled?
* chore: remove specs
* Update app/lib/constants/settings/smtp.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update spec/system/admin/config/admin_updates_smtp_settings_spec.rb
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
* Update app/javascript/packs/admin/config/smtp.js
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
* refactor js as per comments
* refactor as per comments
* Update cypress/integration/seededFlows/adminFlows/config/emailServerSettingsSection.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* refactor: update the Cypress tests
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* 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>
* Removing Articles::Builder making policy decision
This change is a refactoring through triangulation. Given that
`ArticlePolicy#new?` returned true, I'm prepared to assume that calling
`authorize(Article)` in all cases is acceptable.
So to narrow the Builder making a policy decision I renamed the returned
value to reflect what it was actually doing in the logic. And in
renaming, flipped the polarity of the boolean. Why the flip? Because
`needs_authorization == !store_location`.
In consultation with Allison and Jennie, I'm proceeding with a short-cut
to get me unstuck. That unstuck is namely "I need to ensure that the
articles#new action can go through authorization."
Given that I'll be spending time in the authorization layer, I hope
these noted short-cuts and comments will be useful in future spelunking
efforts regarding authorization.
Closes#16529
* Update app/policies/article_policy.rb
Co-authored-by: Michael Kohl <me@citizen428.net>
* Disabling spec
* Update app/policies/article_policy.rb
Co-authored-by: Dwight Scott <dwight@forem.com>
* Update app/policies/article_policy.rb
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michael Kohl <me@citizen428.net>
Co-authored-by: Dwight Scott <dwight@forem.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This is the least effort I can presently think of to allow for us to
reprocess user article's and handle the scenario in which the user may
have once had permission to the liquid tag but no longer.
This DI is not something I imagine using, but may highlight a better
approach for liquid tag permissions.
Related to #12146 and #16460
Prior to this commit, we had a single test for `FeatureFlag.enabled?`,
namely that it delegates to `Flipper.enabled?`. This commit adds some
tests to both verify behavior and communicate to developers the
intentions of the related methods:
- `FeatureFlag.enabled?`
- `FeatureFlag.accessible?`
My hope is the documentation and tests will help address the horrors
mentioned in conversation in #16416:
> I have lived the horrors of having a feature flag accidently
> flipped or incorrectly setup in the first place and half baked
> functionality going live.
Note: I'm not proposing that we test other `Flipper` methods, but
instead to ensure that we're testing and documenting the behavior of
what I believe to be the two primary mechanisms of "putting something
behind a feature flag."
Related to #16406, #16416
* Ensuring we fetch latest podcasts by pubDate
Prior to this commit, we fetched the podcasts that were the first(:limit)
XML `item` nodes in the RSS feed. Some folks might choose to list those
in descending pubDate order, while others list in ascending pubDate
order.
Closes#3580
* Bump for travis
* Added tag search to nav menu
* Added tag search
* Improved tags search results view
* Removed commented lines from the controller
* Fix specs for Search::Tag
* Prepare for tags search pagination
* Fixed Search::Tag specs
* styling
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
* Convert symbol hash keys to strings when calling .perform_async
Fixes a warning from Sidekiq 6.4.0+ about perform_async arguments
which are not equal when passed to perform (`JSON.parse(JSON.dump(arg))`
should equal arg).
This is a safety measure to prevent passing objects (like classes, or
model instances) rather than their representations (like a class name,
or a model's attributes hash).
This warning will be an error in sidekiq 7
* Turn warning into an error in non-production environments
* Use string keys for reaction notification and article fetched
Missed these two on the first pass
* Update example argument hashes for #enqueues_on_correct_queue
Since this calls perform_async under the hood we need to pass json
safe hashes in the test cases as well.
https://github.com/forem/forem/blob/main/spec/workers/shared_examples/enqueues_on_correct_queue.rb
* Convert keys from FollowData#to_h to string before perform_async
I'm not sure enough where else (outside of notification) to_h is being
called, so I'm converting here when building args, rather than in
FollowData#to_h, which might be my next step.
* Let FollowData#to_h return a hash with string keys
Update spec to use string keys as well.
* Make to_h return string keys for ReactionData
Like FollowData, the #to_h method is only used to call
notifications (this is used to enqueue sidekiq jobs).
* Remove a key that was in the hash
Since reaction_data calls to_h, it gets string and not symbol,
keys. Call Hash#except with a key that was actually there.
I saw the following error in Honeybadger:
```
[PROJECT_ROOT]/app/services/notifications/reactions/send.rb:61 :in `call`
old_json_data = notification.json_data
previous_siblings_size = notification.json_data["reaction"]["aggregated_siblings"].size if old_json_data
notification.json_data = json_data
[PROJECT_ROOT]/app/services/notifications/reactions/send.rb:20 :in `call`
[PROJECT_ROOT]/app/workers/notifications/new_reaction_worker.rb:16 :in `perform`
[PROJECT_ROOT]/app/models/notification.rb:84 :in `send_reaction_notification_without_delay`
[PROJECT_ROOT]/app/controllers/reactions_controller.rb:136 :in `destroy_reaction`
[PROJECT_ROOT]/app/controllers/reactions_controller.rb:168 :in `handle_existing_reaction`
[PROJECT_ROOT]/app/controllers/reactions_controller.rb:77 :in `create`
```
As I was exploring the error, I saw that we were making assumptions
about the data coercion. These are valid and somewhat stable
assumptions, but I wanted to look a little deeper into the situation.
This refactor helps consistently negotiate two situations where we're
transforming request-cycle models into lightweight data structures. It
also begins to show a path towards a generalizable "macro" for this behavior.
Related to #2122, #9534
