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-2722 Allow parent skins to be added in wizard #4555

Merged
merged 14 commits into from
Jan 29, 2024

Conversation

weeklies
Copy link
Contributor

@weeklies weeklies commented Jun 24, 2023

  • Fix bug where error 500 is thrown when non-existent parent skin is added

Pull Request Checklist

Issue

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

Purpose

Add a Parent Skin section to the Skin Wizard, as shown in the default editor.

Also, I've fixed a small bug where Error 500 is thrown for non-existent parent skins. I ran into this during testing and didn't find an open Jira issue, so I hope including it in here is OK.

Testing Instructions

See Jira issue.

References

There are open issues regarding Error 500s when editing skins, but this PR doesn't resolve them.

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

Credit

weeklies (she/her)

* Fix bug where error 500 is thrown when non-existent parent skin is added
@sarken
Copy link
Collaborator

sarken commented Jun 26, 2023

Also, I've fixed a small bug where Error 500 is thrown for non-existent parent skins. I ran into this during testing and didn't find an open Jira issue, so I hope including it in here is OK.

That should be fine! Do you have step-by-step instructions for reproducing it that we can add to the issue, just to make sure it gets tested?

@weeklies
Copy link
Contributor Author

Error 500 when adding non-existent parent skin
Instructions:

  • Create a site skin, with a unique name
  • Under Advanced > Parent Skins, add a keysmash site skin (e. g. "keysmashwashere11")
  • Submit the form.
  • Please test for default and Wizard forms.

@weeklies
Copy link
Contributor Author

If it is more convenient, I can leave a comment on Jira.

@sarken
Copy link
Collaborator

sarken commented Jun 27, 2023

Thank you! And you're fine, I copied it right over and into the issue body to make sure it won't be missed.

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.

I think we need some tests for these changes.

Can you add a test for the bug you've fixed in the skin parent model to spec/models/skin_parent_spec.rb?

It looks like we don't have any feature tests for adding parent skins via the interface (everything I saw at a glance looked like it relied on factories) in features/other_b/skin.feature, which I suspect might be because it's excessively difficult due to the JavaScript involved. However, even if it's not feasible to test the entire process of adding a parent skin, I think it would be a good idea to at least add a test to features/other_b/skin_wizard.feature to confirm the parent skin section is there.

app/views/skins/_wizard_form.html.erb Show resolved Hide resolved
app/views/skins/_wizard_form.html.erb Outdated Show resolved Hide resolved
features/other_b/skin_wizard.feature Outdated Show resolved Hide resolved
features/other_b/skin_wizard.feature Outdated Show resolved Hide resolved
features/step_definitions/skin_steps.rb Outdated Show resolved Hide resolved
spec/models/skin_parent_spec.rb Outdated Show resolved Hide resolved
app/models/skin_parent.rb Outdated Show resolved Hide resolved
features/other_b/skin_wizard.feature Outdated Show resolved Hide resolved
features/other_b/skin_wizard.feature Outdated Show resolved Hide resolved
@weeklies weeklies marked this pull request as draft July 16, 2023 00:54
@weeklies
Copy link
Contributor Author

While testing this again, I also found that this PR also fixes another Error 500. Steps to reproduce on production:

  1. Create a new site skin with a unique name (e.g. newskin101)
  2. Add it (newskin101) as a parent skin
  3. See an Error 500 instead of an error message shown when editing a created skin ("You can't make a skin its own parent")

This PR causes the presence validation to run and show an error message. I have changed the name of the attribute from Skin parents parent skin to Parent skin., like so:

image
image

@weeklies weeklies marked this pull request as ready for review July 19, 2023 04:12
@brianjaustin brianjaustin merged commit 2484b9f into otwcode:master Jan 29, 2024
24 checks passed
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.

4 participants