-
Notifications
You must be signed in to change notification settings - Fork 508
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-6058 Hide byline of unrevealed work in notes of child related work #3911
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of these days, I will not press the "Single Comment" button when I mean to use "Start Review." 😩
The title is also exposed in downloads, as I mentioned on Jira. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what's here is good, but it needs the downloads fix ticking mentioned as well.
"A translation of %{work_link} by %{author_link}" : | ||
"Inspired by %{work_link} by %{author_link}" | ||
%> | ||
<% if work.parent.unrevealed? then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't quite work. The downloads currently show either:
A translation of %{work_link} by %{author_link}
Inspired by %{work_link} by %{author_link}
What we're trying to do is -- for unrevealed works -- replace just the %{work_link} by %{author_link}
portion with a work in an unrevealed collection
.
But what you have will write a work in an unrevealed collection
without the words A translation of
or Inspired by
in front of it.
(Also, minor code style note: we typically don't use then
.)
So basically we'd need to divide it up more like:
work_info = if work.parent.unrevealed?
"a work in an unrevealed collection"
else
"%{work_link} by %{author_link}"
end
relation_text = if work.translation
"A translation of %{work_info}."
else
"Inspired by %{work_info}."
end
The tests are failing, but that's understandable... that wasn't meant to be functioning code to copy and paste, just an illustration of how to think about it -- sorry for being unclear! |
<% work_link = link_to work.parent.title.html_safe, url_for(work.parent) %> | ||
<% author_link = byline(work.parent, visibility: 'public', only_path: false) %> | ||
|
||
<%= ts(relation_text, work_link: work_link, author_link: author_link).html_safe %> | ||
<%= ts(relation_text, work_link: work_link, author_link: author_link, work_info: work_info).html_safe %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work when the parent isn't unrevealed? I would have thought I18n would stop after the first substitution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've spoke with Translation, and if it turns out it doesn't work, it would actually be better to have this i18ned like:
<% relation = work.translation ? "translation_of" : "inspired_by" %>
<% if work.parent.unrevealed? %>
<%= t(".#{relation}.unrevealed") %>
<% else %>
<%= t(".#{relation}.revealed", work_link: work_link, creator_link: author_link) %>
<% end %>
Giving Translation this:
inspired_by:
revealed: Inspired by %{work_link} by %{creator_link}.
unrevealed: Inspired by a work in an unrevealed collection.
translation_of:
revealed: Translation of %{work_link} by %{creator_link}.
unrevealed: Translation of a work in an unrevealed collection.
There's still an issue here with the expiration of downloads. IIRC, downloads are cached for a year, and they're only expired when the work is touched (i.e. when (Alternatively, if it's okay that the child work's downloads won't get updated for a year, then it might be good to note that in the issue, which currently says, "Please make sure the work title eventually appears once the work is revealed!") |
Given that I think this is really to stop people creating works only to get the other work information, I am not too worried. However. |
I don't think it would be the end of the world to wait a year if we were only going from unrevealed -> revealed, but like ticking said, we're also looking at revealed -> unrevealed. Those are situations where the creator has, for some reason, decided to make their work no longer available to the public... so yeah, I think it's going to be important to expire the download. |
It looks like we need to handle download expiration and fix the translations, so I'm flipping this back to Action Needed. |
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-6058
Purpose
What does this PR do?
Testing Instructions
See issue.