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
* Select max from a subquery of two things
max(int, int) not a function, says postgres, once it stopped yelling
about parenthesis balancing.
* prefer greatest to select max from subquery
* Ensuring that we don't divide by zero
Adding one to a "always-ish greater than or equal to 0" value. This
should resolve a flakey test and a seemingly erratic production error.
Resolves https://app.honeybadger.io/projects/66984/faults/83596547
* Favoring min over adding 1
Due to syncrhonization antics, let's not assume time is consistent
across servers.
Timey Whimey Wibbley Wobbley
* Update app/services/articles/feeds/weighted_query_strategy.rb
Co-authored-by: Daniel Uber <djuber@gmail.com>
Co-authored-by: Daniel Uber <djuber@gmail.com>
* Ensuring we don't track views of author or unpublished
Prior to this commit, I was surprised to learn that we:
1) Tracked an author's view of their article.
2) Tracked views of an unpublished article.
This came up from a Forem creator asking if they could reset the view
counter. Or trigger the reset on publication.
I think a general business logic policy of don't track views for the
author and don't track views for unpublished articles is a reasonable
default.
Were we to pursue the clear views on publication, we'd need to consider
something that went from unpublished -> published -> unpublished ->
published. Without a more explicit state machine, triggering a
busineiss logic behavior seems a bit unexpected.
In other words, I wrote an article. There are 20 views when I realize
that I need to unpublished it. I make the changes in the unpublished
state, and re-publish. I'd assume that those 20 views would still be
"recorded" and counted towards my article's view counts.
* Adjusting condition structure
Prior to this commit, the `if` clause was rather far to the right. This
helps make the if clause more pronounced.
We are loading the badge for each included tag in the
Search::TagSerializer and seeing warnings from bullet
```
Bullet::Notification::UnoptimizedQueryError:
GET /search/tags?name=ta
USE eager loading detected
Tag => [:badge]
Add to your query: .includes([:badge])
Call stack
/home/travis/build/forem/forem/app/serializers/search/tag_serializer.rb:5:in `block in <class:TagSerializer>'
/home/travis/build/forem/forem/app/services/search/tag.rb:11:in `serialize'
/home/travis/build/forem/forem/app/services/search/tag.rb:7:in `search_documents'
/home/travis/build/forem/forem/app/controllers/search_controller.rb:54:in `tags'
/home/travis/build/forem/forem/app/lib/middlewares/set_time_zone.rb:10:in `call'
```
Follow the advice, now when multiple tags are in the result set,
having multiple badges, badges are loaded only once (at query time)
and not one by one (at serialization time).
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.
* Don't update social information for suspended/banished accounts
* Prevent suspended users/accounts from updating their profile information
* Add tests and fix some logic
