Commit graph

8 commits

Author SHA1 Message Date
Lawrence
0c93bde6c2
Subscribe to Comment Functionality (#19555)
* Move save over and add button labels

* comment notification design and functionality

* controller error handling

* add ternery operator for background class

* minor cleanup

* add proper styling toggles

* tests and mobile spec

* add for all buttons

* add article as a param

* comment is not required on article preview

* update policy spec

* add test for coverage

* add proper button titles

* use the last user comment as comment id

* add better error handling and remove multiple fetch calls in js

* copy change to threads

* update copy, fix multiple fetch calls, add observer for loading more notifications

* update spec

* add rspecs

* more test specs

* fix test specs

* subscribe button text tests

* add export

* optimistically update ui function tests

* button initialize test

* check for comment ancestry

* add feature flag block

* some cleanup

* update logic and specs

* oops

* rubocop issues

* change config names

* change config names

* fix spec

* add one more conditional

* check for comments

* service-object the toggler

* separate routes

* continual refactoring

* refine spec tests

* clean up policy

* Adjust js and view files to match be params

* some service object refactoring

* separate endpoints on client-side

* broken spec

* service specs

* policy spec fixes

* remove action param

* remove action param from spec

* minor updates

* trigger build

* update to post

* Services don't need permitted params

* Notifications continue to be a mess, decorator specs to follow

* Not toggling now

* This doesn't really belong here

* Fix tests after moving

* Streamline front-end

* There could be errors?

* polymorphic_name is a class thing

* ☠️ json_data in notification views

* Use feature_flag helper to get current_user

* Cannot derive current subscription status from notification, only from subscription

* Default is subscribe to all article comments

* Sync up matching unsubscribe, make subscribe idempotent-ish

* Temporarily comment-out button jest tests to see if I can unstick CI

* Stop trying to detect potential subscribe-to-comment

* Stop trying to detect potential subscribe-to-comment

* Minor spec refactoring

* Add specs for article notification decoration

* NotificationDecorator specs are rather long

* Rubocop likes it?

* Minor spec refactoring

* Doesn't need to stay pending

* Add spec for subscription finder

* Expand Subscribe service specs

* YAGNI, but I don't like hard-coded config

* Refactor Unsubscribe service specs

* nobody expects the infinite scroll

* This *might* work, except for feature flag

* Ancestry can be quite deep actually

* Rubocop

* Update subscribe button for subscribe-to-thread

* Jest tests are just tests

* hasAttribute != getAttribute

* Test knows **nothing** about window size

* Tests have been lying about mobileLabel this whole time

* Make payload/endpoint testable

* Is Jest actually happy???

* Still cleaning up jest/eslint

* Temporarily comment-out cypress tests pending feature flag

* Thanks, Rubocop, what would we do without you

* Preserve round-trip for 'top-level' and 'only-author' subscriptions

* Update specs

* Move save button icon

* Update test with better config param

* Try feature flag specific to cypress env

* Tell cypress to wait for (un)subscribing to work

---------

Co-authored-by: Lawrence S <lawrence@forem.com>
Co-authored-by: Joshua Wehner <joshua@forem.com>
2023-07-10 17:11:46 +02: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
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
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
Lewis Sparlin
e621659279
Add Article post_commit when user_id changes with specs (#15132)
* Add Article post_commit when user_id changes with specs

* Add example in requests/admin/articles_spec for updating user
2021-10-25 10:53:21 +07:00
rhymes
10efa25c0a
Add missing presence validators to models (#10495)
* Add missing presence validator for Tag

* Add missing presence validators to Follow

* Add missing presence validators to ProfileField

* Add missing presence validators to Comment

* Add missing presence validators to Profile

* Add missing presence validators to Organization

* Add missing presence validators to Badge

* Add missing presence validators to Webhook::Endpoint

* Add missing presence validators to NotificationSubscription

* Add missing presence validators to PodcastEpisode

* Add missing presence validators to Sponsorship

* Add missing presence validators to PollOption

* Add missing presence validators to Poll

* Add missing presence validators to Article

* Add missing presence validators to EmailAuthorization

* Add missing presence validators to AuditLog

* Add missing presence validators to DataUpdateScript

* Add missing presence validators to User

* Add missing presence validators to SiteConfig

* Fix specs
2020-10-01 16:15:32 +02:00
Ben Halpern
e130400e9e
Finalize UI for all types of notification settings (#3164) 2019-06-14 18:28:38 -04:00
Andy Zhao
b953f17d0c New Feature: Subscribe to posts! (#3149)
* Add MVP of notification subscriptions

* Add rate limiting for notification subscriptions

* Show modal for logged out users on subscribe click

* Add enter key functionality for checkbox

* Add relationships for notification subscriptions

* Add model test for notification subscription

* Deprecated article mutes in favor of notification subscriptions

* Deprecated comment mutes in favor of notification subscriptions

* Move comment muting tests and logic over

* Merge comment and article muting into subscriptions

* Remove redundant check

* Use database to check for rate limit instead of cache

* Test for subscriptions handling notifications properly

* Fix notification model spec for subscriptions

* Fix tests for subscriptions

* Remove rate limiting

* Properly handle logged out users getting subscription status

* Fix test for new pattern

* Update reserved words

* Add config column to notification subscriptions

* Add logic for top level config and move tests

* Test for top level subscribers getting notified

* Fix logic mistake oops

* Add index and refactor comment_user_ids
2019-06-14 12:03:43 -04:00