Skip to content

AO3-7388 Redirect to user skin page with error if trying to preview a skin you can't use#5776

Open
zrei wants to merge 11 commits intootwcode:masterfrom
zrei:AO3-7388
Open

AO3-7388 Redirect to user skin page with error if trying to preview a skin you can't use#5776
zrei wants to merge 11 commits intootwcode:masterfrom
zrei:AO3-7388

Conversation

@zrei
Copy link
Copy Markdown
Contributor

@zrei zrei commented May 1, 2026

Pull Request Checklist

Issue

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

Purpose

Redirects to the user skins page when trying to preview a skin that can't be used but can be viewed while logged in as a user. This includes parent-only site skins and work skins. For skins that a logged-in user cannot view (not owned and not public), they are redirected to their dashboard with an error.

Changed preview to a user-only action. Redirects to the login page with an error when trying to preview any skin while logged out. Redirects to the root page with an error when trying to preview any skin while logged in as an admin.

Testing Instructions

For site skins, refer to Jira.

For work skins:

  1. Log in
  2. Make a work skin
    a. Hi, username! > My Dashboard > Skins > Create Work Skin
    b. Fill in required information
    c. Press “Submit” to save
  3. In your browser address bar, add /preview to the end of the skin URL and press Return

For not owned, not public skins:

  1. Log in
  2. Make a site skin
    a. Hi, username! > My Dashboard > Skins > Create Site Skin
    b. Fill in required information
    c. Press “Submit” to save
  3. Copy the site skin URL
  4. Log into another account and paste the site skin URL into the browser address bar. Add /preview to the end of the URL and press Return

Credit

zrei (she/they)

@zrei
Copy link
Copy Markdown
Contributor Author

zrei commented May 1, 2026

I added a check_visibility call before preview to catch users trying to preview skins they can't view. If I should instead extend the check I've added to the preview action in the skins controller, do let me know!

Copy link
Copy Markdown
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.

Thank you for working on this! Two minor things about the code and then some requests for testing additions

Comment thread app/controllers/skins_controller.rb Outdated
if @skin.is_a?(WorkSkin) || @skin.unusable?
flash[:error] = t("skins.preview.cannot_preview")

redirect_to skin_path(@skin) and return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this should redirect to the user skin page per the Jira issue (so the user's skins index page), not the page of that specific skin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated! For logged-out users, it redirects to the public site skins page. Will update the PR description.

Comment thread app/controllers/skins_controller.rb Outdated
Comment thread spec/controllers/skins_controller_spec.rb Outdated
Comment thread spec/controllers/skins_controller_spec.rb Outdated
it_behaves_like "a non-public skin that cannot be previewed"
end
end
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To cover the (very welcome!) change regarding visibility for the preview action, could you add a variant for an official site skin that is fully accessible and one for a previewable skin by a user with it_behaves_like "an action only the skin author can access"? That way we know that the happy path still works

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The redirect action for the shared example "an action only the skin author can access" is different from the redirect for preview, so I recreated it for the previewable skin by a user.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missed that detail, thanks for the better test changes!

Comment thread spec/controllers/skins_controller_spec.rb
it_behaves_like "a non-public skin that cannot be previewed"
end
end
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missed that detail, thanks for the better test changes!

Comment on lines +716 to +717
let(:notice) { ["You are previewing the skin #{skin.title}. This is a randomly chosen page.", "Go back or click any link to remove the skin.", "Tip: You can preview any archive page you want by tacking on '?site_skin=[skin_id]' like you can see in the url above.", "<a href='#{skin_path(skin)}' class='action' role='button'>Return To Skin To Use</a>"] }
let(:success) { it_redirects_to_with_notice(tag_works_path(tag, site_skin: skin.id), notice) }
Copy link
Copy Markdown
Contributor

@Bilka2 Bilka2 May 6, 2026

Choose a reason for hiding this comment

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

if you wanted to simplify this, you could skip checking the notice with it_redirects_to_simple. It's checked in a cucumber test, so we'd be fine skipping it here

Comment thread app/controllers/skins_controller.rb Outdated
if @skin.is_a?(WorkSkin) || @skin.unusable?
flash[:error] = t(".cannot_preview")

redirect_to skins_path(skin_type: "Site") and return if current_user.nil?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry to change the requirements under you, but you bringing up that we can't redirect to a user's page if they're not logged in made me ask whether we even want to allow guests and admins to preview any skins at all. Turns out we don't! I've updated the Jira issue accordingly, so please update the code to make this action users_only

Copy link
Copy Markdown
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.

Thank you!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants