Prior to this commit, we trusted all user profiles. There are folks
using DEV.to profiles to help with their spam efforts.
With this commit, we're trying to thwart their efforts.
Closesforem/rfcs#334
* Adding tags to data tracing
Prior to this commit, the feed's class was part of the resource. That
resulted in a more opaque view of things.
With this change, we're adding tags to the view so we should have a
better sense of filtering.
* Bump for license/cla test
* Refactoring poll/vote/skip logic
Favor asking the poll if the user has already voted instead of relying
on the poll vote or skip to know about the implementation
details (e.g. should a poll vote know that there's a related model call
poll skipped? likely not)
This refactor also tidies up the specs, for which there's quite a bit of
chatter.
Found via the `flay` gem.
* Adding method documentation
Also need to bump gitpod's build as it halted for the prior commit.
* 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
* initial add of toolbar to v2 editor WIP
* fix Form test knocked out by rebase
* basic image upload functionality added to toolbar
* add cypress tests for toolbar
* move image tests to previous cypress spec
* test markdown insertion from buttons in jest
* small tidy up
* remove right padding in scrollable layout, hide scrollbar
* update tests following new line changes
* add more doc comments
* tweak padding css
* create constant for image placeholder
* add image uploader change missed in staging after merge conflict resolution
* Favoring dependent: :delete_all over :destroy
The `dependent: :destroy` callback is slower than `dependent: :delete`
and `dependent: :delete_all`. We need only favor the `dependent:
:destroy` when there's callbacks that happen.
In the case of :destroy, ActiveRecord instantiates each object and then
runs destroy. Whereas in the case of :delete, ActiveRecord issues a SQL
command to delete the related files.
It is often "safer" to use :destroy, as it guarantees that you'll
instantiate the record and run it's callbacks. But sometimes you have
to go with the speed of SQL.
This relates to #14140. Note there is still more to consider, but given
that we're looking at moving from :destroy on all of an article's page
views to :delete, we might buy enough time in the callback.
* Removing redundancy of article destroy
Prior to this commit, `before_destroy_actions` called the `bust_cache`
method which in turn called `touch_actor_latest_article_updated_at` but
`bust_cache` did not pass the destroying parameter. Then
`before_destroy_actions` immediately called
`touch_actor_latest_article_updated_at` with `destroying: true`.
With this change, we remove one of those duplicate calls.
* Fixing specs regarding relationships
* Fixing specs regarding relationships
* 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>
* Preventing author profile from going transparent
Prior to this change, when you would hover over an author profile that
was "close to" the bottom of the page, the card would be transparent
instead of the expected opaque.
With this change, when you hover over an author profile that is "close"
to the bottom of the page, the card renders as an opaque card.
Now why does this work? I don't really know, except to say that a
`display: table` imperative is a more "chunky" display element than a
block. I would love for someone to explain why this works.
Tested against on MacOS Safari, Firefox, and Chrome.
https://developer.mozilla.org/en-US/docs/Web/CSS/displayFixes#15290
* revert table
* fix
* missed spot
* actions dropdown
Co-authored-by: Paweł Ludwiczak <ludwiczakpawel@gmail.com>
* 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>
* 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
* Adjusts booleans tied to public radio buttons
* Update app/views/admin/creator_settings/_form.html.erb
Revert capitalization of "only" in "Members only"
* Favoring delete_all on user relationships
Prior to this commit, several of the user's "has_many" were marked as
`depenedent: :destroy`.
In the case of :destroy, ActiveRecord instantiates each object and then
runs destroy. Whereas in the case of :delete, ActiveRecord issues a SQL
command to delete the related files.
It is often "safer" to use :destroy, as it guarantees that you'll
instantiate the record and run it's callbacks. But sometimes you have
to go with the speed of SQL.
And while not directly related to #15424 it is representative of our
callback ecosystem creating some unexpected computational loads.
Related to #15442 and #15424
* Noting models that user no longer cascade destroys
* Added alias for app/javascript/controllers
* Added hooks for Stimulus controller.
* Fixed eslint issue with @controllers alias.
* Initial working logo preview.
* Added explicit accept values for png and svg files only for a logo.
* Fixed content layout shift issue and resize to max height 80px.
* Cleaned up logo preview resizing.
* Added focus style to Upload logo label.
* Now the logo preview image has empty alt text as it's visual only.
* Fixed position of upload logo button.
* Removed tooltip for logo.
* Fixed check to load client-side controller.
* Put back tooltip, minus the aria-describedby
* Fixed E2E tests I broke.
* Made the logo preview visible to the accessibility tree.
* feat: update the brand colors on the page when we select a new one
* feat: update the radio button_tags to use crayons-radio
* feat: remove the fill attributes in the svg
* Added support for JPG image upload.
* Fixed height adjustment when width exceeds max preview width.
* feat: add form-background class with an accent
* feat: update the briightness accent on the page
* Fixed JS error if user cancelled file selection for a logo.
* Added the @routes webpack alias for routes.js.erb.
* Fixed data tooltip for assistive technologies.
* Fixed preview logo alignment with upload logo button.
* remove required as it's not doing anything
* feat: update the code brigtness code
* Opting to not show friendly error message if route fails to load.
* Fixed validation message not appearing for logo.
* Revert "Added the @routes webpack alias for routes.js.erb."
This reverts commit 3b6621dcde541f2fa05df6ff75af38955842b88e.
* Reverted to default styling of input[type="file"].
* Moved creator_settings_controller to admin/controllers.
* Updated E2E test for logo preview on the creator settings page.
* create tests for the brand color updates
* feat: update the description
* Update cypress/integration/creatorOnboardingFlows/creatorSettings.spec.js
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
* Update app/javascript/admin/controllers/creator_settings_controller.js
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* feat: do not update branding if an invalid color is provided
* feat: move the brightness code to its own accent calculator file in js utilities
* test: brightness ratios
* chore: remove whitespace
Co-authored-by: Nick Taylor <nick@dev.to>
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
* Guarding against missing experiments
This is a short-circuit to ensure we don't throw exceptions in the
worker. In #15240 we introduce an AbExperiment module that we could use
to insulate against FieldTest implementations.
* Update app/workers/users/record_field_test_event_worker.rb
Co-authored-by: Jamie Gaskins <jamie@forem.com>
* Favoring guard condition over coercion
Co-authored-by: Jamie Gaskins <jamie@forem.com>
* WIP: Add COC and TOS checkboxes to Creator Settings form
* Adds assertions to the creatorSettings.spec.js E2E test
* Removes comments and unnecessary code from Admin::CreatorSettingsController
* Removes params from transaction
* Updates creatorSettings.spec.js to fix checkbox-related failures
* Adds a note to the COC and TOS checkboxes
* 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
* 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
* Removes the 'Setup not complete' banner from the Creator Onboarding flow
* Chains the safe navigation operator to current_user in #creator_setup_mode?
* Replaces current_user with User.with_role(:creator) in creator_setup_mode?
* Checks for URL rather than path helper in #creator_setup_path?
* Consistently remove setup banner on the creator settings form
* Reverts changes to verify_setup_complete.rb
* Adds a setup-banner ID for removal of the gobal setup banner
* Replaces Rspec test with creatorSettings E2E test
* Wraps setup text in regex in creatorSettings.js.spec
* Moving blocked registration domains up
Prior to this commit, the field "worked" but you had to toggle email
authentication to enabled, then fill it out. You could then disable
email notification and it would work.
This builds on #15397
* Update app/views/admin/settings/forms/_authentication.html.erb
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
Prior to this commit, we had filtering options for email registration:
1. Specific allowed domains.
2. Implicitly allow all domains if no allowed domains specified.
With this commit, we add another option: The adminstrator may block one
or more domains from registering.
Closesforem/rfcs#281
This commit removes a handful of magic numbers and instead relies on a
constant.
This has a small impact in that the Basic feed will now return 50
articles instead of 25. However, normalizing the feed pagination window
size helps reduce some oddities in reporting.
In addition, this might be something we consider giving administrators
the ability to set (with default options, because we shouldn't allow
page sizes of 10_000 as that's a massive memory hog).
Related to #14709
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.
