AO3-5742 Fix work_skin_allowed validation never running by pmonfort · Pull Request #5928 · otwcode/otwarchive

pmonfort · 2026-07-11T14:02:41Z

Pull Request Checklist

Have you read "How to write the perfect pull request"?
Have you read the contributing guidelines?
Have you added tests for any changed functionality?
Have you added the Jira issue number as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
Do you have fewer than 5 pull requests already open? If not, please wait until they are reviewed and merged before creating new pull requests.

Issue

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

Purpose

The work_skin_allowed validation was declared with on: :save, which is not a standard Rails validation context. Rails only runs validations on :create and :update by default, so this validation never executed. This allowed a user to apply another user's private work skin by manipulating the form's skin dropdown value via Inspect Element.

This PR:

Removes the on: :save option so the validation runs on both create and update
Uses pseuds_after_saving instead of self.users to check skin ownership, so the validation works for both new (unsaved) and existing works. Only approved creatorships are considered, a pending co-author invitation does not grant access to that user's private skins
Removes the unused custom_work_skin factory (hardcoded work_skin_id: 1)
Rewrites specs to cover: no skin, approved skin, public but not official skin (author and non-author), private skin (author, other user, and pending co-author)

Testing Instructions

Log in as User A
Go to Dashboard > Skins > My Work Skins > Create Work Skin
Create a skin with obvious CSS (e.g. `p { color: re the ID from the URL
Log in as User B
Go to Post > New Work and fill in required fields
Use Inspect Element on the "Select Work Skin" dropd User A's skin ID
Press "Preview" or "Post Without Preview"
Verify you get the error: "You do not have permissi stylesheet."

Credit

Pablo Monfort (he/him)

sarken

A lot of nitpicking, but nothing really major!

sarken · 2026-07-12T02:07:57Z

              at_most: must not add up to more than %{count}. Your work has %{value} of these tags, so you must remove %{diff} of them.
          blocked_gifts: "%{byline} does not accept gifts."
          blocked_your_gifts: "%{byline} does not accept gifts from you."
+          work_skin_not_allowed: You do not have permission to use that custom work stylesheet.


Let's change this while we're here, since the current phrasing is not how we'd normally refer to work skins. Better text would be: "You do not have permission to use that work skin."

Sounds good 👍

sarken · 2026-07-12T04:01:56Z

-    unless self.users.include?(self.work_skin.author) || (self.work_skin.public? && self.work_skin.official?)
-      errors.add(:base, ts("You do not have permission to use that custom work stylesheet."))
-    end
+    return if work_skin.blank?


Could we do return if work_skin.blank? || (work_skin.public? && work_skin.official?) here? I don't think there's any reason to we need to do pseuds_after_saving.filter_map(&:user).uniq if we know the work skin is public and official.

Make sense 👍

sarken · 2026-07-12T04:05:36Z

-    end
+    return if work_skin.blank?
+
+    skin_users = pseuds_after_saving.filter_map(&:user).uniq


I can see where this name is coming from: it serves as a list of users whose skins we should be allowed to use.

However, I think something like work_users, approved_work_creators, or work_users_after_saving might be better -- something like return if approved_work_creators.include?(work_skin.author) feels clearer than return if skin_users.include?(work_skin.author)

sarken · 2026-07-12T04:11:36Z

      end
+    end
+
+    context "when the work skin is approved" do


is public and official might be clearer than approved, especially since the next thing is public but not official.

(Should we also have a test for skins that are official but not public? I'm not 100% sure, but I think it might unfortunately be possible to put a skin in that state...)

Sounds good, I'm going to add test 👍

sarken · 2026-07-12T04:14:45Z

-      let(:work) { build(:custom_work_skin, authors: [@second_author.pseuds.first], work_skin_id: @private_skin.id) }
-      it "cannot be used by another user" do
-        work.work_skin_allowed
+      it "is invalid when the skin author is a pending co-author" do


Could you change this to pending co-creator? Whenever possible in the code, we try to call people who make works "creators" instead of "authors" to be more inclusive of non-textual fanworks.

It would also be a good idea to add a test that confirms the work is valid when the skin author is an approved co-creator. (Users with the archivist role can add co-creators without waiting for approval from the co-creator.)

Make sense, adding the extra test 👍

sarken · 2026-07-12T09:15:39Z

+
+      it "is valid when used by the skin author" do
+        work = build(:work, authors: [skin_author.default_pseud], work_skin_id: skin.id)
+        work.valid?


Instead of calling work.valid? in these examples, could we do expect(work).to be_valid and expect(work).to be_invalid as appropriate?

sarken · 2026-07-12T09:52:37Z

+
+    context "when the work skin is approved" do
+      it "is valid even when used by a different user" do
+        skin = create(:work_skin, author: skin_author, public: true, official: true)


I think create(:work_skin, :public) should work here.

Make sense, thanks!

sarken · 2026-07-12T09:54:58Z

-      let(:work) { build(:custom_work_skin, authors: [work_author.pseuds.first], work_skin_id: @private_skin.id) }
-      it "can be used by the work skin author" do
-        expect(work.save).to be_truthy
+    context "when the work skin is public but not official" do


It probably makes sense to include the same co-creator situations we cover for when the work skin is private here.

Makes sense, adding the test.

github-actions Bot added the Awaiting Review label Jul 11, 2026

pmonfort force-pushed the AO3-5742 branch from 8a4df4e to 431b14c Compare July 11, 2026 14:30

AO3-5742 Fix work_skin_allowed validation never running

b68d9e5

pmonfort force-pushed the AO3-5742 branch from 431b14c to b68d9e5 Compare July 11, 2026 14:41

AO3-5742 Normalize en locale file

20f4427

sarken requested changes Jul 13, 2026

View reviewed changes

sarken added Reviewed: Action Needed and removed Awaiting Review labels Jul 13, 2026

github-actions Bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jul 15, 2026

AO3-5742 Address pull request comments

5e20d2b

pmonfort force-pushed the AO3-5742 branch from 25c24f0 to 5e20d2b Compare July 15, 2026 03:02

pmonfort requested a review from sarken July 15, 2026 03:23

Uh oh!

Conversation

pmonfort commented Jul 11, 2026

Pull Request Checklist

Issue

Purpose

Testing Instructions

Credit

Uh oh!

sarken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmonfort Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pmonfort Jul 15, 2026 •

edited

Loading