* 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
* Require profile fields to belong to a group
Remove the empty group dropdown from the group select tag
Remove the optional: true from the belongs_to validation (raise a
validation error on save if the group is not provided)
Update test to remove belongs_to's optional setting
* Update ProfileFields::Add spec to include profile field group
* update factory to ensure a non-empty profile field group
* Add required profile field group to seeded profile fields
Since profile field is required, create was failing (silently).
Add required field so these are available in e2e tests.
* Use factory `association` to create a profile field group
https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#explicit-definition
* provide required profile field group when posting data
* Fix one spec, drop another
Both of these scripts will be removed in the other branch (changing
the attribute name) - dropping the test here seems lower trouble than
modifying an out of data data update script to pass.
* skip test slated for removal
Similar to the last commit - this test exercises a script about to be
archived in the other branch.
* 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
* figuring things out
* working both ways for issues PRs except disc and PRreview
* working on repo urls
* complete implementation and check specs
* resolve Travis failures
* resolving travis failures
* Fix VCR cassette
* fix regexs
Co-authored-by: Jamie Gaskins <jgaskins@hey.com>
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
This failed in a main build today, testing shows this may give the
same domain very rarely
10_000.times.map do
Addressable::URI.parse(Faker::Internet.url).domain == Addressable::URI.parse(Faker::Internet.url).domain
end.tally
=> {false=>9998, true=>2}
Use a .dev domain (which is absent from faker's internet domain_suffix
list at
https://github.com/faker-ruby/faker/blob/master/lib/locales/en/internet.yml)
to ensure two unique domains are compared.
remove link to faker internals
* 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.
* Refactoring cached_followed_tags
This refactor consolidates two queries and a loop into a single query.
One thing that is unclear is if we're ever really clearing the cache of
the user by id?
What follows is un-related to the refactor but appears to be related to #14937.
> First, I see we set a cache here:
>
> ebdaaaf15b/app/decorators/user_decorator.rb (L23-L34)
>
> But when I look at what busts the user cache, it doesn’t look like we’re busting it for the above cached location:
>
> ebdaaaf15b/app/services/edge_cache/bust_user.rb (L3-L22)
>
> What if anything am I missing in regards to cache busting?
* Adding some deprecated methods
* Updating documentation
* Bump for travis
* Bump for travis
* Updating comments to fix confusion
NOTE: I might be introducing more confusion, but at least
I'm addressing the prior confusion.
* Bump for travis
* Updating so we don't cache ActiveRecord objects
* Selecting only applicable attributes for caching
* Apply suggestions from code review
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Adjusting code based on feedback
* Bump for travis
* Bump for travis
* Update app/decorators/user_decorator.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
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
* 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.
* Don't update social information for suspended/banished accounts
* Prevent suspended users/accounts from updating their profile information
* Add tests and fix some logic
* Refactoring to add helper method
Prior to this commit, we made view level calls to service modules. This
refactor provides convenience methods on the model.
Furthermore, it addresses a few Rubocop violations that "come along for
the ride."
* Ensuring cached entity squaks like User
* Fixing broken spec
* Fixing typo
* Modify welcome notification generator to handle errors per-message
The original flow aborted all processing when any
ActiveRecord::RecordNotFound exception was raised. This causes a
situation where a missing broadcast message (by title) causes that
notification message to fail, and each day the same failure to
occur (since it wasn't successfully sent the day before), blocking all
further messages.
We want these to proceed on the schedule implicit in the checks, and
failing early prevents that.
Modify the error handling to catch RecordNotFound on each message, and
continue throught the checks until a notification is enqueued or all
checks have been attempted.
* Run welcome broadcast check 4 hours after the user was created
This started to fail when I modified the generator (why? because the
user was 7 days old and _other_ notifications would be sent).
Ensure the user is 4 hours old before checking that the disabled
welcome broadcast message is checked, and not sent.
Prior to this commit, we had two places that need to know the nuances
ofquerying for tag flares and what we should include in our queries for
serialization.
With this commit, we're factoring towards a common source of knowledge
and providing a much needed test for the expected output of this
serialization.
Loosely related to #15916, #15983, #15994, and #16032.
* add needed fields to tag search
* specs
* add badge to tags suggest response (#15840)
* Add badge to tags suggest response
* Update tags_spec.rb
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
* limit tag search badge to only badge_image
* only unpack badge image if badge exists
Co-authored-by: Dwight Scott <dwight@forem.com>
Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.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
* 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?
* 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
* Remove modify_hr_tags method definition
this breaks things that call it.
* Remove direct uses of modify_hr_tags fixer method
Remove from class METHODS lists, remove test cases about this behavior, and remove from
methods lists in test cases.
* Remove hr tag modification spec
Still don't understand what problem this was fixing, but I've removed
the test case.
* Remove reactions to a user when deleting the user
Fixes#15245
---
I have no idea why `create(:vomit_reaction, reactable: user)` gave a
validation error about category being invalid, but `build().save`
worked fine.
* Move relation details to reaction scope `for_user`
Add a user method to access this by passing self to reaction scope
* Create a moderator when flagging user in factory
This might be a missing requirement in the reactions factory - but
this suppresses an error about invalid category I was getting when
setting a privileged reaction on the user via
create(:vomit_reaction, reactable: user)
Everywhere else it looks like we do this with `user: moderator` (where
there's a trusted user in the surrounding context).
* Move trusted user requirement into vomit_reaction factory
* Add a data update script to remove reactions to deleted users
This cleans up the remaining invalid references and should make the
change in #15249 obsolete (i.e. we don't need to limit the view to
exclude items that no longer exist).
* Use the feed source url when the feed url (from links) is empty
This probably should just be using @feed_source_url always to make a
base path for absolute paths without a hostname, rather than expecting
this from the feed.
An example why we should not be using @feed.url would be
individual posts at domain/slug but the "index" page at
domain/blog (with a generic screen on /), and images hosted from
/assets/ without /blog/ anywhere in the asset path.
Partial fix for #15488
* Use item.url as base_url for relative links
It's possible the rss feed and the contained articles are served from
separate hosts/subdomains/paths. Don't assume the entries are related
to the feed or the feed_source_url, use the item's url for relative
links in that item.
* Update failing spec to add url to item double
* Add two guards when purchasing credits
This prevents sending an empty card to stripe, and avoids a
foreseeable error when purchasing 0 credits. Error returned from
stripe is "This value must be greater than or equal to 1".
* Move redirect back into action method
Only use validation methods to check input and set appropriate messages.
* Accept organization id if set
The payment service will use the organization id (and charge the
organization) when that's the purchaser. Don't fail when no stripe
token or card selected if an organization admin is purchasing credits.
* Move validation into the payment processing service
This cleans up the number of details the controller needs to worry
about
Now only if the process payment service had any tests...
* Prevent only organization_id from permitting purchase
and move this check to a method, with an intention revealing name
* Add i18n translation for error message
There weren't any service side error messages - this will be displayed
in a view (credits/purchase) but comes from the payment service -
other messages (from stripe) are passed through as-is there.
If this is the wrong _place_ for the translation I can retool.
* Add unit test for payment processing
Only checking errors are raised as expected and the internal error
message is translated.
* Leave translation alone
Revert test translation
Prior to this fix, in the production rails console we ran the following:
```ruby
Articles::Feeds::WeightedQueryStrategy
.new(user: nil, page: 1, tags: nil)
.featured_story_and_default_home_feed(user_signed_in: false)
```
The result was: `[nil, []]`. Which in the
[StoriesController#assign_feed_stories][1] is where we have the
following:
```ruby
@featured_story, @stories = feed
.featured_story_and_default_home_feed(
user_signed_in: user_signed_in?)
```
In Blazer, I ran the following query at 2021-12-02 Thu 20:41 EST:
```sql
SELECT * FROM articles
WHERE featured = true
AND published = true
AND published_at > (NOW() - interval '7 days')
AND published_at < NOW()
AND main_image IS NOT NULL
```
So the logic of filtering with `only_featured` resulted in the
`@featured_story` being `nil`. Which then cascaded into the logic for
determining the remainder of the results.
For further discussion see the inline comments and specs that I wrote.
In addition to the above, I'm also reusing
`Articles::Feeds::FindFeaturedStory`
Prior to this commit, the `Articles::Feeds::WeightedQueryStrategy` first
grabbed the featured story, then grabbed the remaining articles. This
is different than the implementation of the
`Articles::Feeds::LargeForemExperimental` in which we first grab all the
articles then claim one as the featured.
In the WeightedQueryStrategy, the articles would not include the
featured. However, in the LargeForemExperimental it would. This
results in upstream logic in [app/javascript/articles/Feed.jsx][2]
going through and removing one of the articles.
[1]:c1a3ba99eb/app/controllers/stories_controller.rb (L237-L271)
[2]:c1a3ba99eb/app/javascript/articles/Feed.jsx (L57-L63)
* Fixing logic error for feed
Prior to this change, we were forcing features into the feed.
* Adding spec to help ensure parameter match
Prior to this commit, but before I had merged changes into main, the I
had implemented `alias default_home_feed call`. However, this broke the
interface assumed in the feeds controller.
* Avoid email reassignment when connecting oauth provider
* Use less intrusive approach (we still want to preserve <provider>_username assignment)
* Add spec to avoid regressions
* Fixing weighted query hotness grab logic
Prior to this commit, I carried over (albeit imprecisely) logic from the
LargeForemExperimental. That logic was to help limit articles to those
that were published since the user's latest page view.
However, I introduced a bug in this transcription. Now both
LargeForemExperimental and WeightedQueryStrategy use the same logic to
determine the oldest publication date to search for in the feed.
This resolves a bug reported where users were not seeing a large number
of items in their feed.
Incidentally, if a forem has little activity in the 18 hours, there
might be very few items in the feed.
I believe, going forward, we may need to better parameterize how many
hours is considered "stale since last page view".
Related to #15240
* Fixing implementation detail
* Extracting helper method
Prior to this commit, I had introduced a method an put it in a less
ideal module space. This commit extracts that method to a more readily
shareable module space.
I've added a few more specs to help clarify and verify behavior.
* Updating documentation
* Renaming and documenting variables/constants
* Fixing that which I broke
* 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>
* Limiting feed queries
Prior to this commit, we were joining on all comments. With this
change, we're limiting the comments to reflect the same time period as
our article limit.
Hopefully this further improves the performance of the query.
* Removing unintentional puts statement
* Fix Rpush app when creating Android notification
* Include Android devices in Service spec
* Apply suggestions from code review
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
