* 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
* WIP: add a creatore settings form
* WIP: updat the controller to use the Creator Settings FOREM
* feat: use the creator settings form for the new action
* feat: add some default values for the new action
* a note about form data
* update the initiaize function to set some default values
* feat: update the form to use the model data
* feat: permit adn use the attributes within creator_settings_form
* update the flash error
* refactor: require and permit parameters
* chore: use booleans, set defaults and validate the form
* spec: update all the creator_settings tests
* chore: remove comment
* refactor: use self
* feat: aggregate failures'
* chore: remove the logo uploader in the controller
* refactor: update error handling
* feat: update the wasy the controller handles success and error
* chore: remove the resource errors
* feat: show flash message on new line
* fix: use a redirect so that we can get back to /new
* refactor: pass these values through as they seem to be caching whne setting them as default
* chore: change default values
* spec: update tests
* Fix CreatorSettingsForm specs
* fix: use a boolean for public
* spec: add another test for the success var
* fix: radio button labels to correspond + cyress specs
* spec: update based on new changes
* spec: update the params and the expected output
* spec: update the comments and status
* feat: no need for the initialize as we use Active Record Attributes
* feat: update the tac and coc to be persisted when ticked
* fix: amend spec
* blank space
* Message
Co-authored-by: Michael Kohl <me@citizen428.net>
* User decorator (and spec) should use `trusted?`
Fixes a few issues seen in an rspec run that show as:
DEPRECATION WARNING: User#trusted is deprecated, favor
User#trusted? (called from config_body_class at
/opt/apps/forem/app/decorators/user_decorator.rb:58)
And here:
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:112
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:121
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:130
/opt/apps/forem/spec/decorators/user_decorator_spec.rb:139
* prefer User trusted? to trusted
DEPRECATION WARNING: User#trusted is deprecated, favor
User#trusted? (called from permissions at
/opt/apps/forem/app/models/rating_vote.rb:25)
* Prefer trusted? to trusted in user spec
* Use warned? rather than warned in admin article partial
* use trusted? rather than trusted in moderator requests spec
* Prefer trusted? to trusted in moderations controller
* Prefer trusted? to trusted in moderations view
* User auditable? should call trusted? and not trusted
Deprecations go rolling right along.
* Invert guard clause logic to be positive
The original "return unless multiple negated conditions hold" guard
was cumbersome.
Invert to return if any of the exceptions apply, namely:
- this is a comment or readinglist rating (rather than explicit),
allowed for all
- this rating is from a moderator/trusted user (allowed)
- this rating is offered by the article's author (allowed)
I had intended to also remove the safe navigation operators (since it
wasn't clear why there would be a null user or null article, as
rating_vote joins users to articles with a score), but the builtin
validation tests (is expected to validate ...) build objects with
missing attributes, and raise errors when the spec is run.
We're revisiting the behavior of the feed scoring, and the selection
of articles will be less heavily influenced by the article's score.
This test was nested within a group related to active campaigns, but
the minimum score is a user experience setting (so if we want to test
for that it should happen outside the context), and minimum article
score will likely not be a reliable criterion for filtering the home
feed in the future.
Remove this example (it was skipped because failing), as it adds
little value and is at a minimum misplaced (wrong surrounding
context), at worst it's wrong or likely to be wrong soon.
* Removing the stackbit integration
The feature didn't quite work and Stackbit no longer supports this
integration.
Yes there are a few places where the webhooks has a string of "stackbit"
but I'm hesitant to remove that, as they are the part of the Webhooks
tests.
Closes#15700
* Update lib/data_update_scripts/20211206222716_remove_stackbit_page.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
I think this is related to some requests triggering the weighted query
field test (which changes the rules for the home feed in a way that
may include these campaign tags and handles relevancy differently from
the scoring).
It's reasonable to revisit these. Either the entire context should
include an assertion that all field tests return the original
strategy, never weighted, or we should determine their importance if
we expect the weighted query strategy to be universally deployed (or
widely deployed).
* 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
* 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
* window.ForemMobile namespaced functions
* Fix broken Buildkite
* Wait for data-loaded before using Runtime in pack
* Bring back sprockets base
* Better error handling
* Smal fixes to ConsumerApp to support Android platform
* Fix typo
* utilities/waitForDataLoaded.js
* Update app/javascript/utilities/waitForDataLoaded.js
Co-authored-by: Nick Taylor <nick@forem.com>
* Review feedback
* Fix failing spec
* Cleanup promise
* Remove old utility file
* Add Cypress check for namespaced availability
* More specs
* Refactor to rely on ForemMobile for native bridge messaging
* Fix spec/requests/api/v0/instances_spec.rb
* Fix devices spec
* Remove changes to /api/instance
* Refactor to dynamic import
* Fix typo
* Fixed custom event detail payload for tests.
* mock ForemMobile function instead of webkit call directly
* failing jest test debug
* Another attempt
* Fixed broken test that was missing an onMainImageUrlChange prop on the component.
* Fixed mobile bridging E2E tests.
* Move Cypress tests to mobileFlows
* Add JSDocs + cypress spec for injectJSMessage when logged out
* Cleanup + Disable native image upload until AppStore approval
Co-authored-by: Nick Taylor <nick@forem.com>
Co-authored-by: Nick Taylor <nick@dev.to>
* Updating valid domain registration
Prior to this commit, our regular expression did not account for the `-`
character as valid within the domain. The `-` character cannot be the
first nor last character of the domain (e.g. `-hello.com` nor
`hello-.com` are invalid but `hell-o.com` is valid).
In addition I tidied up the default value of the
`blocked_registration_email_domains` to match it's sibling `allowed_registration_email_domains`.
This relates to the spammer seo-hunt.com
* Adding validator domain validator spec
* Moving request spec to unit spec
This change does two things:
1) Allows for 2 character domains
2) Moves the spec for 2 character domains from an expensive spec to a
cheaper to run spec (e.g., request-cycle to unit-test validator)
It preserves PR #12268
* 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>
Don't allow passing `tag[name]=` in post params to admin tag update.
There's no field for this in the view, it's not expected that someone
change the name (it serves as a natural key) - it's preferrable to
alias an old name to a new tag if a renaming is desired.
* Add a failing test for comments index of deleted article
After https://github.com/forem/forem/pull/15052 removed the (also
broken) deleted commentable template and the redirect, requests for
comments from deleted articles raise no method errors (initially
trying to set the page title to "Comments for commentable.title" but
the assumption that @commentable is non-nil is present in several
other places.
This test currently fails (by design). Either we want to update the
controller so that comments from deleted articles are no longer
viewable (returning not found, as we did prior to
https://github.com/forem/forem/pull/5199 or making the view safe for
the case where @commentable is nil and no @root_comment is present.
These requests are happening quite frequently (@maestromac and I
suspect the sitemap may contain these pages and crawlers are visiting
the site from a published link), as evidenced by
https://app.honeybadger.io/projects/66984/faults/81994265
* Add request spec for deleted article scenario
Additionally, relabel the legacy spec (updated during #15052) to
clarify we do not render the deleted_commentable_comment view (this is
testing the comment.path, rather than the article.path/comments index
The scenario with comment.path sets `@root_comment` in the controller,
so does not trigger errors on the `@commentable` method calls.
* Update view
pass 1: get the errors to stop (need to check the rendered page is
also usable, the comment tree is not shown when commentable is nil and
this might be a huge usability/correctness issue).
* extract logic from comments index to methods and update tests
We no longer expect deleted article's comments index to render (should
return 404), only direct links to comments of deleted articles.
Only assign @article in the index for the view if it is in fact an
article (not a podcast episode, it's possible `@root_comment` was for
a podcast episode).
* remove unneeded (and soon to be incorrect spec)
No longer want or need to test for the case where root comment is nil
and so is commentable. This was an exploratory spec (scaffolding) and
can be removed.
* Undo nil safety changes to comments index
and fix typo in comments spec
* views/stories and home related i18n
* PR key names fixes
* remove ja.yml
* Update en.yml
* Update fr.yml
* Update articles_search_spec.rb
* Update _meta.html.erb
* Update index.html.erb
* Update _meta.html.erb
* Update articles_search_spec.rb
text in HTML should not contain raw brackets
* Update articles_search_spec.rb
* Update _signup_modal.html.erb
* Add back missing Search text
Co-authored-by: Fernando Valverde <fernando@visualcosita.com>
* Handling hiding/collapsing hidden comments client side
* Updated comment quality text
* Added placeholder for comments against podcast episodes
* Added logic for encompassing co-author-ids in articles
* Removed rspecs validating non-presence of hidden comments in dom
* Fixed hiding flow on comments page for a commentable
* Fixed e2e specs
* Addressed feedback on e2e tests
* first pass of styling of the page
* feat: scroll to the position in the config controller in stimulus
* feat: add the tooltip and a cursor thats not allowed
* feat: add a disabled property for all that needs smtp to be enabled
* feat: update the form styling
* feat: update the form
* specs: update the label name
* fix: syntax error
* chore: update the newline
* spec: test the invitation flow
* fix test
* feat: update the form as per suggestions
* spec: update the tests to match the new workflow
* oops committed debugging code
* chore: add a before to set the smtp_enabled method
* feat: update the boldness and fontsize of the link
* feat: remove size
* videos ui updates
* move cheese around
* .
* .
* thumbnail yolo
* thumbnail yolo
* object fit
* aspect ratio yolo
* .
* loading videos
* Make Travis and asset pipeline happy with .mp4 extension
* Use Cloudinary video url, testing for Travis
* Try weird things
* Try a real image url
* Use a real image instead of 'video' string
* Update views.scss
Co-authored-by: Andy Z <17884966+Zhao-Andy@users.noreply.github.com>
* tag edit
* more
* chop chop
* spec
* Apply suggestions from code review
Co-authored-by: Michael Kohl <me@citizen428.net>
* french translation
* Fix I18n usage
* Apply suggestions from code review
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* lang
Co-authored-by: Michael Kohl <me@citizen428.net>
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Fix and clean up podcast pages
* Adjust tests
* Fix a test
* Fix missing i18n
* Fix tests
* Fix tests
* Fiddle with test
* Sure up css and tests
* Add featured as allowed param
* Fix a couple tests
* xit out test
* Update app/views/podcast_episodes/index.html.erb
* Update app/views/podcast_episodes/index.html.erb
Co-authored-by: ludwiczakpawel <ludwiczakpawel@gmail.com>
Co-authored-by: ludwiczakpawel <ludwiczakpawel@gmail.com>
* When a user is blocked from commenting on an article, show correct error message
* Created new error type ModerationUnauthorizedError and changed error text for it
* Move hard-coded Tags index copy to [en,fr] locale files
* Use tags.pluralize for a better page title implementation
* Add better french translations
* Once more fr translation fix
* Remove .pluralize from title i18n
* Consolidate and internationalize various 'contact us' prompts
* Update spec/requests/user/user_settings_spec.rb
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
* Only prevent merge user if 2+ more identities, not 1+ more
* Refactor error messages into constants
* Skip only if no identities on deleted user
* Raise standard error instead of a string
* Add test to confirm correct identity was merged
* tokenize settings menu and add French translations
* add quotes around tokens
* add tokens in alphabetical order
Co-authored-by: Ben Halpern <bendhalpern@gmail.com>
