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.

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

AO3-4820 AO3-3810 Validate skins with "archive" in title #4771

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

Conversation

weeklies
Copy link
Contributor

@weeklies weeklies commented Mar 28, 2024

Issue

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

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

Purpose

  • Resolve AO3-3810 and AO3-4820
  • Remove an unnecessary step in skin_steps
  • Fix failing tests

Note: I am not sure which admins should be allowed to edit a skin with “Archive”.

Testing Instructions

See both issues.

app/models/skin.rb Outdated Show resolved Hide resolved
@sarken
Copy link
Member

sarken commented Mar 29, 2024

Note: I am not sure which admins should be allowed to edit a skin with “Archive”.

It should depend on the type of skin -- superadmins can edit site or work skins, and support admins can edit work skins only.

MANAGE_SITE_SKINS = %w[superadmin].freeze
MANAGE_WORK_SKINS = %w[superadmin support].freeze

@weeklies weeklies marked this pull request as draft March 29, 2024 09:09
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 pointed out a few code style things that would be nice to change.

It looks like reviewdog got a bit confused with the lines in this file for some reason (pointing out issues for code that didn't change), which might get fixed if you merge master into this branch. But the Style/ParenthesesAroundCondition violation that hound points out should indeed be fixed.

app/models/skin.rb Outdated Show resolved Hide resolved
app/models/skin.rb Outdated Show resolved Hide resolved
features/step_definitions/skin_steps.rb Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Has Migrations Contains migrations and therefore needs special attention when deploying label Apr 16, 2024
@weeklies
Copy link
Contributor Author

I added a migration to make the skin title index unique as Rubocop suggested, as it seems that would be beneficial for efficiency.

@weeklies weeklies marked this pull request as ready for review April 23, 2024 14:00
@Bilka2
Copy link
Contributor

Bilka2 commented Apr 23, 2024

Just dropping a note here that I have run out of knowledge to continue reviewing this PR in particular, someone else will have to review the functional code. But the locale looks good :)

@brianjaustin brianjaustin self-requested a review May 8, 2024 14:15
config/locales/models/en.yml Outdated Show resolved Hide resolved
config/locales/models/en.yml Show resolved Hide resolved
title:
taken: must be unique
invalid_media: We don't currently support the media type %{media}, sorry! If we should, please let Support know.
no_public_preview: You need to upload a screencap if you want to share your skin.
Copy link
Member

Choose a reason for hiding this comment

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

Noting that we would update the wording here, but at some point this feature is going away anyways so it's not really worth updating only to delete later

app/models/skin.rb Outdated Show resolved Hide resolved
Copy link
Member

@brianjaustin brianjaustin left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Migrations Contains migrations and therefore needs special attention when deploying Reviewed: Ready to Merge
Projects
None yet
4 participants