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-5578 Use ActiveStorage for existing image uploads #4807

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

AO3-5578 Use ActiveStorage for existing image uploads #4807

wants to merge 26 commits into from

Conversation

brianjaustin
Copy link
Member

Pull Request Checklist

Issue

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

Purpose

Migrate existing places that use kt-paperclip to ActiveStorage (pseud, collection, and skin icons)

Credit

Brian Austin (they/he)

@brianjaustin brianjaustin added the Has Production Config Changes Modifies the config file and needs special attention when deploying label May 6, 2024
@github-actions github-actions bot added Has Migrations Contains migrations and therefore needs special attention when deploying Gem Updates Awaiting Review labels May 6, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems May 6, 2024
View reviewed changes
app/models/skin.rb Dismissed Show dismissed Hide dismissed
app/models/skin.rb Dismissed Show dismissed Hide dismissed
@brianjaustin
Copy link
Member Author

Because ActiveStorage doesn't support prefixes within buckets well, we'll need to update the Ansible local config to separate things out IMO that's better anyways, since we currently co-mingle some production and non-production things... 😬 There's probably a way to do this with less friction, but I think this should be OK since we're copying things over

@Bilka2
Copy link
Contributor

Bilka2 commented May 6, 2024

Because ActiveStorage doesn't support prefixes within buckets well, we'll need to update the Ansible local config to separate things out IMO that's better anyways, since we currently co-mingle some production and non-production things...

Looks like #4738 (AO3-6324' comments) agree with you.

Bilka2
Bilka2 reviewed May 15, 2024
View reviewed changes
app/models/pseud.rb Outdated Show resolved Hide resolved
Bilka2
Bilka2 reviewed May 25, 2024
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

I had a look at all the code except for the rake task.

The fixtures in test/fixtures/pseuds.yml have (empty) lines for icon_file_name, icon_content_type, icon_updated_at and icon_file_size that should probably be removed.

I think there should be a followup Jira issue to remove the icon_file_name, icon_content_type, icon_updated_at and icon_file_size columns from the database for the affected classes.

app/models/collection.rb Outdated Show resolved Hide resolved
app/models/pseud.rb Outdated Show resolved Hide resolved
app/views/challenge_claims/_unposted_claim_blurb.html.erb Outdated Show resolved Hide resolved
features/step_definitions/icon_steps.rb Show resolved Hide resolved
Bilka2
Bilka2 reviewed May 25, 2024
View reviewed changes
app/helpers/users_helper.rb Outdated Show resolved Hide resolved
Bilka2
Bilka2 suggested changes May 26, 2024
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.

A few final comments. The rake task looks good, as does the rest of the code.

I also tried a few things in local dev, including looking at various icons in various places, uploading a pseud icon and using that to test the validator and resizing. Resizing acts the same as on production.

app/models/collection.rb Show resolved Hide resolved
app/models/comment.rb Show resolved Hide resolved
app/validators/attachment_validator.rb Outdated Show resolved Hide resolved
Bilka2
Bilka2 reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

I think I found how to fix the tests.

app/controllers/collections_controller.rb Outdated Show resolved Hide resolved
spec/requests/blocked_users_n_plus_one_spec.rb Show resolved Hide resolved
spec/requests/muted_users_n_plus_one_spec.rb Show resolved Hide resolved
spec/requests/people_n_plus_one_spec.rb Outdated Show resolved Hide resolved
@brianjaustin
Incomplete fixes (expected fail)
0c82e49 
At this point, I'm a bit at a loss for how to make mutes/blocks not generate n+1.
This should do something, but it ends up doing nothing???
brianjaustin
brianjaustin commented Jun 28, 2024
View reviewed changes
app/controllers/collections_controller.rb
@@ -25,11 +25,23 @@ def load_collection_from_id

def index
Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up deleting the N+1 spec for this method, as it was flagging something unrelated and we want to move collections to Elasticsearch anyways (also I'd like to keep the PR from getting too much out of hand 🙈)

@brianjaustin brianjaustin requested a review from Bilka2 June 28, 2024 18:05
Bilka2
Bilka2 reviewed Jun 28, 2024
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

I looked over everything again and mostly have some comments on the scopes! Thank you for the work on the N+1 query prevention and specs, those were more annoying than I had expected.

app/controllers/blocked/users_controller.rb Outdated Show resolved Hide resolved
app/controllers/collections_controller.rb Outdated Show resolved Hide resolved
app/controllers/muted/users_controller.rb Outdated Show resolved Hide resolved
app/controllers/skins_controller.rb Outdated Show resolved Hide resolved
app/controllers/collections_controller.rb Outdated Show resolved Hide resolved
app/models/collection.rb Outdated Show resolved Hide resolved
app/models/pseud.rb Outdated Show resolved Hide resolved
spec/requests/pseuds_n_plus_one_spec.rb Show resolved Hide resolved
spec/requests/people_n_plus_one_spec.rb Show resolved Hide resolved
spec/requests/people_n_plus_one_spec.rb Outdated Show resolved Hide resolved
Bilka2
Bilka2 reviewed Jun 29, 2024
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Just some more nitpicks, then this is finally done!

app/controllers/collections_controller.rb Outdated Show resolved Hide resolved
app/controllers/muted/users_controller.rb Outdated Show resolved Hide resolved
app/controllers/blocked/users_controller.rb Outdated Show resolved Hide resolved
Bilka2
Bilka2 approved these changes Jun 29, 2024
View reviewed changes
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

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

@Bilka2 Bilka2 Bilka2 approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Gem Updates Has Migrations Contains migrations and therefore needs special attention when deploying Has Production Config Changes Modifies the config file and needs special attention when deploying Priority: High Reviewed: Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brianjaustin @Bilka2 @sarken