* 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>
* AB Test for Feed Strategy
Adding the plumbing for performing AB Tests on the feed strategy.
You can add the environment variable `AB_TEST_SERVICE_FEED_STRATEGY` to
specify the field test to run.
For example, from the forem application home directory you can run the
following to force a strategy for all users:
```console
AB_TEST_SERVICE_FEED_STRATEGY=original bin/start
```
Forcing the strategy can significantly help with quality assurance testing.
* Adding configurable weighted feed strategy
I wrote about this exploration on [DEV.to][1]. This particular
implementation provides significant configuration options for A/B
testing.
At it's core this `Articles::Feeds::WeightedQueryStrategy` provides a
means to query articles that are relevant to the user. The
configuration helps us test and refine those values.
Further more this implementation introduces the idea of some common
interfaces for feed strategies. I'm not happy with the naming
convention but we can work to change that.
This even hints at a mechanism for administrators to configure.
Related to forem/rfcs#314, as I work towards understanding the feed
algorithm. And #15180
[1]:https://dev.to/jeremyf/practicing-postgresql-and-postulating-improvements-5m
* Renaming AbTestService to AbExperiment
I'm loath to end any production class's filename with "test". I don't
want minitest or rspec coming along and attempting to "test" that file.
In addition, I find that Experiment better describes the concept.
Also, per conversations with Michael K, we favor nouns objects to go
in app/models and verb objects to go in app/services. That's a great
separation.
* Factoring AbExperiment into a Controller concern
I wish this could be a stand-alone module but the implementation of
field test has cases that require controller context.
* Sanitizing configurable weighted strategy cases
* Minor refactoring of coding structure
* Renaming parameter for clarity
* Renaming variable to avoid confusion
Prior to the commit, `env` is a controller variable. The injected `env`
variable didn't collide, but did create just a bit of confusion.
With this change, I'm trying to disentangle from that variable.
* Factoring AbExperiment away from mixin concern
Per discussion with Michael K, we both agree that we do not like
controller concern mixins. They can quickly complicate the legibility
of any given codebase.
This refactor provides a common and simplified interface for a Plain Old
Ruby Object (PORO) that accounts for the implementation considerations
of the underlying "field_test" controller method.
* Adding Datadog.tracer.trace around feed actions
* Adding featured_article_factor
Prior to this commit, there was no consideration for how moderators
engage with an article. This is a first nudge towards that effort.
* Removing spec
Add a spec that says yes or no we're feature testing something seems
fragile. Namely, why are we wanting to assert that as a truism.
* Helping ensure we're "tracing" the same things
From the code comment:
> Hey, why the to_a you say? Because the
> LargeForemExperimental has already done this. But the
> weighted strategy has not. I also don't want to alter the
> weighted query implementation as it returns a lovely
> ActiveRecord::Relation. So this is a concession.
* Addressing parameters for tracing function
* For weight feed query only count positive tags
A user assigns explicit points to the tags that they do or do not want
to see. This change treats the negative explicit_points as an indicator
that they don't want to see the tag.
* Adding weighted factor for privileged user reactions
* Updating comments to better clarify
* Adding additional constraint for published_at
* Updating documentation and adding logger
* Adding documentation concerning method call
* Upping logging level for experiments
See conversations in https://github.com/forem/forem/pull/15240
* Adding group by clauses to appease postgres configs
* Injecting relevant scoring method's joins
Prior to this commit, we included all of the joins regardless of the
scoring methods chosen. With this commit, we only inject joins that are
used with the commit.
What this would allow us to do is to then have some pre-cooked scoring
methods but not turn them on. Or to turn off the more expensive scoring
methods.
* Adding feature flag for feed strategy experiment
* Ensuring reading list excludes unpublished articles
Prior to this commit, if a given user adds an article to their reading
list then the author unpublishes the article, the article remains in the
reading list. When the given user would then "click" on the now
unpublished article, they would get a 404 Not Found notice.
With this commit, we now exclude unpublished articles from the search
results for the reading list.
This should also address the issue of attempting to archive the reading
list item (because it won't be visible in the listing).
One of the behaviors that is expected is if, in the above scenario, the
author again publishes the article, that article will again "appear" on
the reading list for the given user.
Closes#14796
* Updating documentation and adding spec
* Addressing pull request feedback
Yes, I should've used a scope! And also, no need to test present. Just
let it's "truthy"-ness speak for itself!
* Update spec/services/search/reading_list_spec.rb
Co-authored-by: Jamie Gaskins <jamie@forem.com>
Co-authored-by: Jamie Gaskins <jamie@forem.com>
* Enable Forem (Passport) Auth
* Remove feature flag DUS
* Add prompt for Forem Passport in admin
* Add spec & fix broken one
* Link to docs instead of passport site
* Parameterizing featured story requiring main image
This begins to ease the resolution of #15292.
If we merge #15240, I could see setting an `Article` constant for this
value and allowing the administrator to choose the particular behavior.
Once we have insight from the product team, we can move forward with a
more comprehensive solution.
* Adding default parameter
While this is an `@api private` method, I am aligning the defaults with
it's `@api public` caller. Yes, only specs call the method, but I'd
rather not fiddle with the specs at this moment in time.
* Reworking logic to be more scannable
* Guarding against spam from OAuth Sources
Prior to this commit, when an administrator had indicated blocked email
domains, those blocks were not applied to identities created via the
OAuth sources (e.g., Twitter, Facebook, etc). With this change, we're
hooking into the similar logic flow as suspended email accounts.
Related to #15403, #15397, and forem/rfcs#281
* Adding class documentation to exception
* Update app/services/authentication/authenticator.rb
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
* Remove Connect
* Remove more Connect specs
* Remove a lot more Connect code
* 🚮
* It all has to go
* Explicitly add httpclient
* Update application layout
* Remove messages association from User
* Start fixing specs
* reintroduce util function and refactor references
* Remove Connect Cypress test
* Fix more specs
* Remove Connect from listings
* Ignore contact_via_connect column on listings
* Remove contact_via_connect usages
* Ignore mod_chat_channel_id on tags
* Drop Connect tables
* Remove email_connect_messages from user notification settings
* Re-add httpclient 2.8.3
This was mistakenly removed as a merge conflict
* Don't need to exclude removed chat channel file
* Remove unneeded style for chat channels
* Remove unneeded channel list prop type
* Remove chat channels index/connect-link from getPageEntries
* Re-add comment from httpclient in Gemfile
* Remove connect references from mailers
Tag Moderators no longer have a chat channel
No longer will users be notified about new messages (there won't be
any)
No longer will users be notified about channel invites (you can't
invite anyone anymore)
* Don't configure Pusher and remove PUSHER_* from .env_sample
since it's removed from gemfile, the Pusher constant will not resolve, if this is
configured in the environment variables we'll fail to boot.
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Dan Uber <dan@forem.com>
* 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
* Disable all providers when providers_to_enable param is blank
* Remove guard, a couple more spec examples
* Update app/services/settings/authentication/upsert.rb
Co-authored-by: Fernando Valverde <fernando@visualcosita.com>
Co-authored-by: Fernando Valverde <fernando@visualcosita.com>
Prior to this commit, there existed duplicate logic between two of the
primary user feeds. This refactor introduces a new object which is
solely concerned with calculating scores to add to an article base score.
With this commit, we can introduce A/B testing by changing passing
different config values to `ArticleScoreCalculatorForUser.new`.
My suspicion is that for some of this, we might be able to better
leverage the database via select statements and SQL sums. However, that
is presently outside of what I'm prepared to tackle.
* 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>
* Removing state change and unused method
Prior to this commit, the `@comment_weight` value would change. This is
not ideal as depending on the call sequence, can notably change the
output. I suspect this state change occurred so as to not alter an
underlying spec.
What this change does is remove the state change, removes a dead method,
renames a method (to the dead method name), and leverages
parameterization to better test a spec that was brittle based on
possible state changes.
I believe, from a logical stand point, that this change does not impact
the functionality nor the actual logic that is part of the production
call path.
* Marking methods as @api private
The goal is to highlight that we really shouldn't be calling these
outside of their contained class. Ideally, I'd love to make them
private methods, but there are specs and would prefer to not use
`__send__` to change those specs.
This is a noop change.
* Renaming method to refelct returned param order
Prior to this commit the "default_home_feed_and_featured_story" returned
an array of `[featured_story, default_home_feed]`. The method name and
the order of the returned values were misaligned. This change helps
align the method name and the order of those returned values.
Note, methods such as `each_with_index` have an `element, index`
parameter order.
* Fix notifications not deleting because mentions already destroyed
* Fix spelling in spec
* Use rspec expect/to assertions for delete notification spec clarity
* resolve failing spec, and remove need for environment check 🎉
* Fix rotation of images uploaded from iPhones
iPhones[1] take pictures in portrait mode with the orientation set to
90ºCCW. We noticed that uploading one of these images to a Forem
instance caused the image to be rotated when rendered. The current
theory is that Imgproxy is not maintaining the orientation value when
processing, which is solved by setting `auto_rotate: true` (serialized
into the Imgproxy URL as `ar:1`), according to the documentation.
See: 8e6585e28d/docs/generating_the_url_advanced.md (auto-rotate)
[1] This probably happens with other phones, but we discovered the issue
using an iPhone.
* Fix Imgproxy URL expectation for auto_rotate
* Remove check_box type from profile fields
* Fix profile field spec
* Delete emphasized_comments_experiment.rb
This file was not supposed to be part of this PR
* Initial forem omniauth strategy setup work
* Finish basic raw proof of concept
* Some playing around
* use OAuth payloads + PASSPORT_OAUTH_URL for local dev
* Use FeatureFlag for Forem Passport Auth
* Working on tests
* Fix tests 🤞🏼 & some cleanup
* Use correct namespace within lib directory (match class namespace)
* Test to ensure Forem Passport auth is restricted by FeatureFlag
* Add broadcast + work on tests
* Update spec/lib/data_update_scripts/insert_forem_connect_broadcast_message_spec.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Hash format
* Schema cleanup + inline comments
* Use temprorary Heroku domain
* More cleanup
* Missed one
* Back to passport.forem.com
* Require correct path in lib
* Apply suggestions from code review
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Use with_indifferent_access for symbol hash access in Forem strategy
Co-authored-by: benhalpern <bendhalpern@gmail.com>
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
