Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AO3-6711 Add a "Delete All Subscriptions" option to the Subscriptions page #5096

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

AO3-6711 Add a "Delete All Subscriptions" option to the Subscriptions page #5096

wants to merge 10 commits into from

Conversation

marcus8448
Copy link
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6711

Purpose

  • Adds a button to the "My Subscriptions" page that allows for the mass deletion of a user's subscriptions.
  • When viewed from the "My Work/Series/User Subscriptions" pages the button will only delete the subscriptions of the given type.

Credit

marcus8448 (he/him)

sarken
sarken reviewed Mar 10, 2025
View reviewed changes
Copy link
Collaborator

@sarken sarken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I had two quick notes, but please feel free to wait to action them until someone has done a more thorough review!

@Bilka2 Bilka2 mentioned this pull request Mar 16, 2025
Merged
Bilka2
Bilka2 reviewed Mar 17, 2025
View reviewed changes
Bilka2
Bilka2 reviewed Mar 17, 2025
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, these were meant to be all review comments, not single comments.

This looks really good, thank you! I just had some small notes for the tests and one discrepancy in the text compared to the Jira issues.

Bilka2
Bilka2 reviewed Mar 17, 2025
View reviewed changes
app/controllers/subscriptions_controller.rb Outdated

success = true
@subscriptions.each do |subscription|
subscription.destroy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subscription.destroy
subscription.destroy!

The other one won't throw if there is an issue, it'll just return false.

@marcus8448
Copy link
Contributor Author

Thanks for the feedback!

I've updated the code to pluralize and downcase the type parameter. Previously, visiting subscription pages with a capitalized or singular type parameter (e.g. ?type=Users) would cause translations to break (although realistically this will never occur without manual URL editing).

Side note: I think there may be bugs in the error handling of the clear history feature, as @errors doesn't seem to exist, and (assuming destroy works the same as subscriptions) it will never throw anyways (readings_controller).

Bilka2
Bilka2 approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

Side note: I think there may be bugs in the error handling of the clear history feature, as @errors doesn't seem to exist, and (assuming destroy works the same as subscriptions) it will never throw anyways (readings_controller).

Thanks for pointing this out, I've raised this for discussion internally.

Update: https://otwarchive.atlassian.net/browse/AO3-6953 created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarken sarken sarken left review comments

@Hamham6 Hamham6 Hamham6 left review comments

@Bilka2 Bilka2 Bilka2 approved these changes

Assignees
No one assigned
Labels
Reviewed: Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcus8448 @sarken @Bilka2 @Hamham6