AO3-7397 Configurable page for skin previews#5744
AO3-7397 Configurable page for skin previews#5744nicolacleary wants to merge 3 commits intootwcode:masterfrom
Conversation
nicolacleary
force-pushed
the
AO3-7397_skin_preview_page
branch
from
779a5ad to
c92baf3
Compare
| flash[:notice] << t(".skin_title", title: @skin.title) | ||
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) |
There was a problem hiding this comment.
Rubocop would complain on the usage of ts when I removed "This is a randomly chosen page" from the notice.
I decided it made sense to convert all of the lines rather than just one
| # e.g. /works -> http://test.archiveofourown.org/works?site_skin=[skin_id] | ||
| # Production should pick something more suitable with roughly 1-20 works and | ||
| # predictable content such as /users/OTW_Translation/works | ||
| SKIN_PREVIEW_URL: '/works' |
There was a problem hiding this comment.
The ticket asks for /users/OTW_Translation/works but this user is not present in the development data. I chose /works for this value since it should always be available, but this should be changed the proper value in the production config.
There was a problem hiding this comment.
this user exists on staging (the test.ao3.org site linked in the issue, for testers who have access to it) and you can create it in your local environment for testing it yourself (see Creating Development Data in the wiki)
There was a problem hiding this comment.
I understand that it's possible to create it locally, and I have successfully created users in my dev environment.
I worry that configuring a user not in the dev data could negatively impact the dev experience:
- In order for the preview function to work in dev, devs will have to manually create a user or update the config
- If this happens with multiple features the dev setup instructions become complicated and hard to follow and maintain
- It won't be clear that it's a config error and not a bug
I could:
- Add the
OTW_Translationuser in the fixtures - I believe this would then be part of the dev/test data for new installations - Configure an existing dev user here - the config would still need to be changed before testing/prod
- Override the config in
development.rb- I think this is possible but I would have to check
I am not sure what would be the preferred approach would be here. If the config.yml file shouldn't be changed before testing then naturally I can adjust it. Cheers
There was a problem hiding this comment.
oh yeah good point, defaulting to a page that would exist with the dev data makes sense
setting a user works page by default as an example for people making their own archives is probably a good idea, but idk if option 1 or 2 is better (probably not 3, i don't think using the .rb files to override these configs like that is done)
maybe ask bilka since he made the ticket?
There was a problem hiding this comment.
Not a full review, but yes please change the URL here to the URL desired in the issue without further changes to dev data. It will simply 404 in dev, which isn't an issue - this is not a commonly used page so I don't expect a significant impact to the experience there.
It vastly simplifies the timing/organisation of testing and deploying if we don't have to fit in config changes between that, so we prefer to use production-ready values in this file
There was a problem hiding this comment.
Clear, thanks both! I have updated the config as asked
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) | ||
| redirect_to "#{ArchiveConfig.SKIN_PREVIEW_URL}?site_skin=#{@skin.id}" |
There was a problem hiding this comment.
This provides no guarantees that the url is correct (formatting or exists) - e.g. if you configure "/idonotexist" then you would get an error only when you tried to preview a skin.
3 participants
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-7397
Purpose
Redirect to the URL specified in the config with
SKIN_PREVIEW_URLwhen previewing a skin.Remove the functionality to redirect to the works index of a random tag - this provided no guarantees on the kind of content shown and would sometimes choose tags that no longer existed (resulting in 500 errors).
Credit
nicolacleary (she/her)