* 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
This is a hypothetical and experimental interface change. The goal of
the change is to begin looking at what a "Feed"'s method interface would
look like.
I would like to reduce the controller logic necessary for deciding on
different strategies, and instead defer to a builder/strategy lookup
pattern.
* 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.
* Start cleaning up feed code
* Move published_articles_by_tag to service
* Add timeframe and latest feed services
* fixup! Move published_articles_by_tag to service
* Move constant to correct location
* Fix spec
* Add SiteConfig.feed_pinned_article and validation
* Display pinned article at the top of feed
* Add (basic) functionality to pin/unpin post
* Admins can pin other users posts as well
* Hide the button if looking at the non pinned post
* Add pinned/unpinned snackbar message
* Rename SiteConfig usage to Settings::General
* Add pinned article to the Admin articles index
* Show the pin post button when there's no pinned article
* Move pinning to a separate controller
* Fix SiteConfig reference
* Hide PinController actions to unauthorized users
* PinnedArticlesController#show action and refactor some of the code
* Add Modal interaction
* Fix modal-pinned checkbox interaction
* Fixed pin/unpin post
* Add ArticleDecorator#pinned? specs
* Add PinnedArticlePolicy and PinnedArticlesController specs
* Add ability to actually pin an article from the admin after submit
* Add partial Cypress pin/unpin spec
* Fix pinned article and add basic Cypress interaction tests
* Add Crayons styling to modal
* Only render the pinned article on the default Feed page
* Use persisted?
* Add some comments
* Update app/javascript/articles/Article.jsx
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/javascript/packs/homePageFeed.jsx
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Fix Cypress tests
* Update app/javascript/admin/controllers/article_controller.js
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
* Fix pinning in article show page
* Used PinnedArticle domain model
* Fix spec
* Update cypress/integration/adminFlows/articles/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/adminFlows/articles/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/adminFlows/articles/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/adminFlows/articles/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/adminFlows/articles/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/articleFlows/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update cypress/integration/articleFlows/pinArticle.spec.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Update app/views/admin/articles/index.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Fix merge woes
* Add missing article pin post flows
* Add missing admin article flows
* Add Unpin to Admin as well
* Add Audit::Log entries for pin/unpin actions
* Update app/controllers/stories/feeds_controller.rb
Co-authored-by: Michael Kohl <citizen428@dev.to>
* Do not rate limit in E2E tests
* Use .find instead of .filter
* Rename ArticleIdValidator to ExistingArticleIdValidator
* Treat draft and deleted articles the same
* Make sure posts can be pinned after the pinned article is unpublished or deleted
* Use .get directly
* Fix spec and fix PinnedArticlesController#show
* Strengthen pinArticle Cypress tests
* Add Cypress test heading guard
* Add another Cypress test heading guard
* Remove duplicate validator
* Try using the Tools: header instead of the article title
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Michael Kohl <citizen428@dev.to>
* Initial field test spec work
* Clean up specs
* Fix codeclimage
* Cleanups
* Update names
* Rename and remove need to pass logged in status
* Leadup to prod: two experiments
* Move test logic to controller and add tests
Co-authored-by: Josh Puetz <josh@grorichpuetz.com>
* Add json support at stories/index endpoint
* Tweak tests
* Add main image
* Some light refactoring in StoriesController
* Tiny test refactor
* Split feed generation to it's own controller
* Use Jbuilder for article formatting
* Blank lines thanks Rubocop
* Don't cache this response: we want it generated per use each time
* Whoops!
* PR changes and tweaks to Timeframmer constants
* Remove initializer test since arguments aren't required
