Commit graph

749 commits

Author SHA1 Message Date
Ridhwana
5e93d3a25e
Add a contact email address to the /admin/customization/config (#16497)
* feat: remove the default email and cobine the periodic digest and the contact email under the Email section

* refactor: rename  the email_link to contact link and use the contact_email as a default and fallback to the ForemInstance.email

* chore: alignment

* feat: use the contact_email helper

* feat: move the contact_email to the ForemInstance model

* feat: use ForemInstance.contact_email instead of the application helper method

* removed the application Helper

* feat: set the dafault on the contact_email

* fix: cypress tests

* Update app/lib/constants/settings/general.rb

Co-authored-by: Michael Kohl <me@citizen428.net>

Co-authored-by: Michael Kohl <me@citizen428.net>
2022-02-15 17:37:08 +02:00
Daniel Uber
afa214ac52
Require profile fields to belong to a group (#16390)
* 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.
2022-02-15 09:27:18 -06:00
Arit Amana
2842bc68d6
Implement Unified Embeds for Github Repo, Issue, Pull-Request and Comment URLs (#16422)
* 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>
2022-02-07 16:38:43 -05:00
Jeremy Friesen
30a894a6eb
Removing deprecated User methods (#16435)
* Removing deprecated User methods

Related to #15762, #15691, and #15624

* Bump for travis
2022-02-07 10:32:58 -05:00
Daniel Uber
99307167f7
I18n's short format for times differs from "%b %e", update test (#16414)
This passed in the branch because the last commit was on a two digit
day (so the digit padding wasn't needed or expected, it checked "Jan 30" ==
"Jan 30", but today "Feb 4" != "Feb  4")
2022-02-03 15:49:09 -06:00
yheuhtozr
e45536af37
app/models i18n (#16124)
* app/models etc i18n

* delete ja.yml

* fix for PR review

* fix for spec

* delete ja.yml

* fix for spec updated
2022-02-03 13:41:42 -05:00
Anna Buianova
9a11beec50
Reorganized article published_at/dates specs (#16403) 2022-02-03 19:04:58 +03:00
VISHAL DEEPAK
14a945326d
Add boolean attribute main_image_from_frontmatter to indicate if cove… (#16075)
* Add boolean attribute main_image_from_frontmatter to indicate if cover image was set via frontmatter

* use article model spec for main_image_from_frontmatter test cases

* Add data migration script and spec for main_image_from_frontmatter for articles

* Update app/models/article.rb

Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>

Co-authored-by: Jeremy Friesen <jeremy.n.friesen@gmail.com>
Co-authored-by: Michael Kohl <me@citizen428.net>
2022-02-03 09:16:48 -05:00
Anna Buianova
c2ce2c32d5
Improved article date specs (#16395)
* Improved article date specs

* Say rejects
2022-02-03 10:07:42 +03:00
Ben Halpern
96a935ffec
Add new cases for reaction weights to lead to more effective rankings (#16138)
* Base author reaction functionality

* Get logic more in place

* Finalize tests

* Add admin clause for new user points

* Add proper registered_at for seeded user

* Fix regsitered_at in e2e

* Add registered_at across the board in e2e tests

* Update comments

* Update spec/models/reaction_spec.rb

Co-authored-by: Jamie Gaskins <jamie@forem.com>

* Update spec/models/reaction_spec.rb

Co-authored-by: Jamie Gaskins <jamie@forem.com>

* Update spec/models/reaction_spec.rb

Co-authored-by: Jamie Gaskins <jamie@forem.com>

* Put points in constant and refactor points resave logic

Co-authored-by: Jamie Gaskins <jamie@forem.com>
2022-01-28 12:28:18 -05:00
Jeremy Friesen
77d9640e3e
Initial statement of intent regarding Listings (#16335)
* Initial statement of intent regarding Listings

I want to use this pull request as a means of conveying intended
direction; namely that we want Forem administrators to be able to toggle
off (or on) the Listing feature set.

This refactor is a first, yet incomplete pass, that doesn't make the
code worse.  Expect more of a similar vein as we work to put the Listing
feature set behind a feature flag.

Related https://github.com/forem/rfcs/issues/291

* Adding feature flag based on reviewer comment
2022-01-28 10:07:06 -05:00
Jeremy Friesen
0484c550c5
Create/update by NavigationLink identity (#16312)
Prior to this commit, we were treating all NavigationLink attributes as
unique.  So, were we to change a position of one of the NavigationLinks
during the add_navigation_links rake task, we would have created a new
NavigationLink (and the only one would have remained).

With this commit, we're introducing the concept of the NavigationLink's
surrogate identity, that is to say if we have two NavigationLink objects
with the same `url` and `name` we should consider them the same
NavigationLink.  This allows us to update properties of those
NavigationLinks (via the rake task) without the risk of creating new
entries.

This unblocks PR #16268 which addresses #16076.
2022-01-27 13:51:41 -05:00
Michael Kohl
67f68b1cc2
POC: use Sidekiq.perform_bulk for Github repos (#16293) 2022-01-26 09:11:07 +07:00
Michael Kohl
31c805aa09
Let's be friends again, Rubocop (#16294) 2022-01-25 11:58:00 -05:00
Jeremy Friesen
2b3f4e1342
Refactoring cached_followed_tags (#16175)
* 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>
2022-01-24 19:53:48 -05:00
Ben Halpern
4960f8913f
Refactor AB Experiments and exercise all variants in rspec (#16236)
* Refactor AB Experiments and exercise all variants in rspec

* Fix test that still uses wut
2022-01-24 17:03:58 -05:00
Mac Siri
3f2653a14d
Prefer custom SMTP config over Sendgrid (#16216) 2022-01-24 14:11:50 -05:00
Michael Kohl
a0edc9ac7e
Add Settings::Base.to_h (#16241) 2022-01-21 22:23:24 +07:00
Jeremy Friesen
fc8158a5ba
Stripping tags from "tags.short_summary" column (#16248)
On the DEV.to database, I ran the following SQL:

```sql
SELECT name, short_summary FROM tags WHERE short_summary LIKE '%<%';
```

There were three tags with a `<` in them:

- changelog
- cnc2018
- javascript

This PR will tidy up our short summary as part of saving a tag.  It will
also tidy up the existing tags.
2022-01-21 09:38:26 -05:00
Jeremy Friesen
711f1bb0cd
Let override of ToS and CoC show in onboarding (#16217)
* Let override of ToS and CoC show in onboarding

Prior to this commit any changes to the terms or code of conduct were
not reflected in the onboarding links.

With this commit, I'm leveraging a newly created method that first
checks if there's a page for the given slug.  If there is, use that
page's copy.

To test:

- Overwrite default /terms page on a Forem
- Navigate to /onboarding or sign up as a new user
- Click on Terms of Use link text
- Review Terms of Use

Closes #15296

* Adding spec around not passing a block
2022-01-20 19:45:39 -05:00
Ben Halpern
8159f4383c
Feed experiment 4: Final order (#16128)
* Feed experiment 4: Final order

* Remove development line

* Fix text in tests

* Fix text in tests

* Switch query to greatest of
2022-01-17 17:05:43 -05:00
Jeremy Friesen
3253ba2c7a
Patching ERB rendering of the data-info JSON (#16067)
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
2022-01-14 08:30:49 -05:00
Josh Puetz
629c7114da
Login with Google (#15986) 2022-01-13 10:25:52 -06:00
Jeremy Friesen
05bad3ae10
Replacing custom call with existing cached method (#16083)
There's a few things going on:

1. I introduced [a change][1].
2. There was a [data script][2] that should've completed successfully, but
   some data was not converted (see [Blazer query on DEV.to][3])

Thus the current state of the data means that we have have serialized
data in an `OpenStruct` format and `Articles::CachedEntity` format.

This patch should work because the OpenStruct should previously have an
`image_profile_90` attribute.

In addition, I have added some recommendations and tests to better
describe what's happening.

[1]:9780dba380/app/models/articles/cached_entity.rb (L4)
[2]:9780dba380/lib/data_update_scripts/20200723070918_update_articles_cached_entities.rb (L1)
[3]:https://dev.to/admin/blazer/queries/609-serialized-data-in-mixed-forms
2022-01-13 07:48:32 -05:00
Jeremy Friesen
b115b2d17e
Appeasing Rubocop as it sneaks some changes in (#16085)
I was working on another branch and as part of my commit, Rubocop
removed a validation (but not the spec that asserted the validation).

Below is the "non-updating" rubocop offense on the other branch.

```shell
❯ rubocop ./app/models/notification_subscription.rb
Inspecting 1 file
C

Offenses:

app/models/notification_subscription.rb:13:29: C: [Correctable]
Rails/RedundantPresenceValidationOnBelongsTo: Remove explicit presence
validation for notifiable_id.
  validates :notifiable_id, presence: true
                            ^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable
```

To remediate, I ran:

```shell
> rubocop --only "Rails/RedundantPresenceValidationOnBelongsTo" \
  --auto-correct
```

This resolved the `app/models`.  Then did some regex magic and removed
the assertions from `spec/models`.

For Forem folks, I wrote a [forem.team post][1] discuss if this is how
we want to proceed.

[1]:https://forem.team/jeremy/rubocop-auto-updating-mayhem-33a6
2022-01-13 07:48:01 -05:00
Jeremy Friesen
6929cd908c
Moving from 'should' syntax to 'expect' (#16084)
In Travis I found the following message:

> Using `should` from rspec-expectations' old `:should` syntax without
> explicitly enabling the syntax is deprecated. Use the new `:expect`
> syntax or explicitly enable `:should` with
> `config.expect_with(:rspec) > { |c| c.syntax = :should }`
> instead. Called from
> /home/travis/build/forem/forem/spec/models/html_variant_spec.rb:76:in
> `block (2 levels) in <main>'.
2022-01-13 00:33:42 -05:00
Jeremy Friesen
0a1b222bb7
Moving the "Null" user object closer to User (#16070)
* Moving the "Null" user object closer to User

Prior to this commit, we had the presentation concept of a DELETED_USER
in the ApplicationHelper.  Further, we did type checks against that
object instead of relying attributes of the object.

With this commit, I moved the "Null" user closer to the User definition
to help highlight the concept that there might be deleted users.

I didn't remove all of the type checks, but did attempt to create a more
"duck-type" object.

Further, I moved away from an OpenStruct which in the past (and perhaps
present) had performance issues.

* Moving DeletedUser into Users module space
2022-01-12 08:45:01 -05:00
Ben Halpern
294b3471af
Proposing feed experiment 3: More comment count weight (#15993)
* Initial work

* Implement strategy injection into WeightedQueryStrategy

* Modify field_test config and adding variants

* Change orginal to constant and make some other adjustments

* Fix hardcoded test values
2022-01-11 11:05:06 -05:00
Michael Kohl
1c80304fd5
Remove htmlentities gem (#15958) 2022-01-06 21:14:32 +07:00
Jeremy Friesen
b56ae3acbb
Extracting constants and tidying tests (#15946)
Prior to this commit, we had a couple of references to magic strings
regarding DisplayAdEvent objects.  This commit seeks to consolidate that
behavior.

In a handful of cases we had javascript variables, but as these JS files
are `erb` templates, we can inject the constant as well.  This reduces
repetition of knowledge.
2022-01-05 09:19:39 -05:00
Jeremy Friesen
6269aa4108
Proposing a new feed experiment (#15789)
* Proposing a new feed experiment

This commit involves tweaking a few subtle aspects of the challenger:

1) Remove the sort by published date on the relevance score.  The
   articles will now be listed in the order of perceived relevance.
2) Disable a few levers that did little.  There really aren't any
   `articles.featured = true` articles in the database.
3) Gently increase the weight of each comment in a linear manner.
4) Give a little more weight to posts that don't have followed tags.

In addition, it involves renaming the conversions to better convey their
implementation.  This renaming helps create more descriptive labels for
the test results at `/admin/abtests`.

This commit also adds logic to repurpose the AbExperiment feed_strategy
logic; I envision adjusting the weighted feed strategy levers with some
frequency.

Per discussions, I'm also disabling the "not logged in" feed testing.
We'll use the LargeForemExperimental for this.

* Flipping attribute name to positive

Mentally "not disabled" is harder to parse than "enabled".  This change
helps with setting an optimistic attribute.

* Bump for travis

* Bump for travis

* Removing file committed by mistake

* Extracting method
2022-01-03 14:38:14 -05:00
Derek Crosson
ae27d15be7
Prevent whitespace unicode characters from being used in article title and tag (#14909)
* test: whitespace unicode characters cannot be used as titles or tags

* feat: add localized error message

* refactor: use localized error message

* fix: whitespace unicode characters cannot be used as titles or tags

* chore: fix locale after merge

* refactor: fix indentation

* Fix spelling of prohibited in method names

Additionally, rubocop removed a redundant user_id validation since
Article belongs to user.

* Fix spelling

Missed one method call on the same line (fixed the first of two
instances needing changes)

* add failing test cases

The reorganization to remove let was due to a limit on nesting rspec
contexts (it inside context inside describe inside describe when
trying to use different titles in let blocks in contexts)

The first test was clarified (the "U+202D" string that looks like the
code for a unicode point is valid, but the character \u202d is
expected to be invalid

The remaining two tests are based on the feedback I'd given in the PR,
failing because we don't assert title is present after removing
unicode whitespace, and we don't actually set the title (gsub is
non-destructive, returning a value).

* Set title to sanitized title

contains_prohibitied_unicode_characters? was using == (which is not a
good match for strings to regexes), use =~ instead

Change invalid example characters in titles from \u202d (bidi override) to
\u200a (hair space)

Assert that the empty title is blank and can't be blank (even though
it contains no prohibitd characters after replacement)

* change the definition of prohibited characters

From the description, it looks like "unicode space characters" was the
desired rejection set. It was unclear what the existing regex was
matching on (I couldn't get the expected examples to match correctly).

Given the intent, I select "unicode space property, except the ascii
space character", which may _also_ be incorrect but passed the tests.

* Remove now-invalid expectation for presence of user id

Since this was redundant (belongs_to user), we no longer will validate
presence of user id, and we should not test for it.

* Only reject bidirectional text controls

The original issue pointed to the BIDI controls as problematic.

While they called out other "whitespace" characters, as we accept a
wider range of input languages, the need to handle non-printing
punctuation (for example a group separating space for some asian
languages).

It's possible a wider list of characters should be added - if that's
the case I suggest this regex and the sanitization be moved to a
standalone class, and each of the recommendations in
https://www.w3.org/TR/unicode-xml/#Charlist be checked for
applicability.

* Check for blank title after sanitizing disallowed characters

Since validate_title modifies the title, and doesn't set error
messages on the model itself, check for non-empty title _after_
potentially removing any disallowed characters.

* Remove invalid characters before validation

Remove the validate_title and contains_prohibited_characters methods,
and center all logic on the remove prohibitied unicode characters
method.

Remove unneeded and unused arguments (input string is always title,
replacement is always removal/empty string).

* handle case when validating null title

If the title's nil, we don't want to call match? since it will fail.

Title will sometimes be nil when validating.

Co-authored-by: Ben Halpern <bendhalpern@gmail.com>
Co-authored-by: Dan Uber <dan@forem.com>
2022-01-03 09:37:03 -05:00
Anna Buianova
792cd68786
Fixed setting Identity#auth_data_dump in seeds and factory (#15874)
* Fixed setting Identity#auth_data_dump in seeds and factory

* Mock omniauth providers in the Identity spec
2021-12-28 09:02:15 +03:00
Michael Kohl
06d6b68d9d
Remove webhooks and related code (#15827) 2021-12-28 10:56:37 +07:00
Jeremy Friesen
a40efc6bbd
Refactoring questions asked of user (#15762)
* 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
2021-12-21 12:45:12 -05:00
Michael Kohl
872b007c30
Remove Doorkeeper gem (#15749) 2021-12-21 12:29:58 +07:00
Anna Buianova
9f688be406
Moved setting old_username from before_validation to Users::Update (#15782)
* Moved setting old_username from before_validation to Users::Update

* Removed unused code

* Fixed specs after moving setting old_usernames
2021-12-20 11:08:09 +03:00
Jamie Gaskins
9e0570968e
Strip surrounding whitespace from HTMLVariant name (#15776) 2021-12-14 14:42:06 -05:00
Daniel Uber
9f9594ccdc
role deprecation cleanup (#15717)
* 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.
2021-12-09 15:27:45 -06:00
Jeremy Friesen
ec615fa377
Blocking subdomains of domains marked as spam (#15715)
Closes #15714
2021-12-08 14:08:52 -05:00
Josh Soref
0403e78f08
Spelling (#15702)
* spelling: access

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: additional

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: administrative

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: aggregate

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: assigns

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: attributes

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: autocomplete

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: because

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: between

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: bootstrap

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: calculating

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: captcha

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: character

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: chosen

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: commenter

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: competitor

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: componentize

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: contrast

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: corresponding

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: description

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: destroyed

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: destroys

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: discussion

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: episode

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: escaped

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: evaluates

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: expired

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: explicitly

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: facebook

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: fragment

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: functionality

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: improper

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: incentive

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: interfere

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: latest

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: message

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: minimum

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: moderator

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: mouseover

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: mutual

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: nonexistent

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: notification

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: occasionally

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: occurrence

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: occurs

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: octokit

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: offset

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: omitted

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: opacity

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: organization

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: organizations

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: overridden

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: override

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: overriding

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: prefill

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: previous

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: profile

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: recycling

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: registered

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: repositories

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: rescuing

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: response

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: returns

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: second

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: separator

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: services

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: subscriber

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: subscription

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: success

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: successful

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: successfully

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: suppress

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: test

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: thought

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: uniqueness

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: unknown

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: unproductive

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: unreachable

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: unsuccessful

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: utilities

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: utility

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: valid

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: voluntarily

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: vomited

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: website

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

* spelling: withholding

Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>

Co-authored-by: Josh Soref <jsoref@users.noreply.github.com>
2021-12-07 06:59:10 -07:00
Jeremy Friesen
29f6853ee9
Refactoring Spam Handler (#15412)
* 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>
2021-11-30 12:45:12 -05:00
Jeremy Friesen
25f0681c4f
Validating community does not include < nor > (#15482)
If a community name has a < or > it may cause havoc when the community
attempts to send an email.

Closes #15274
2021-11-29 10:55:02 -05:00
Jeremy Friesen
2a26fa3538
Refactoring poll/vote/skip logic (#15441)
* 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.
2021-11-29 10:52:32 -05:00
Jeremy Friesen
816e92e549
Configurable weighted feed strategy (#15240)
* 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
2021-11-29 10:46:56 -05:00
Jeremy Friesen
be97f2fc07
Favoring dependent: :delete_all over :destroy (#15442)
* 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
2021-11-24 13:56:16 -05:00
Fernando Valverde
75a722bd6e
ForemMobile namespaced functions (#15212)
* 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>
2021-11-23 15:25:29 -06:00
Fernando Valverde
c384755bae
Enable Forem (Passport) Auth (#15141)
* 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
2021-11-23 14:59:00 -06:00
Jeremy Friesen
ee0a521a83
Favoring delete_all on user relationships (#15443)
* 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
2021-11-23 11:50:12 -05:00
Michael Kohl
09828853f6
✂✂✂ Remove Connect (#14734)
* 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>
2021-11-18 08:21:00 -06:00