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-6058 Hide byline of unrevealed work in notes of related work #4607

Merged
merged 32 commits into from
Oct 29, 2023

Conversation

weeklies
Copy link
Contributor

@weeklies weeklies commented Aug 15, 2023

Apologies for the numerous PRs, I promise this is the last for a while.

Pull Request Checklist

Issue

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

Purpose

  • Do not reveal parent or child works in unrevealed collections
  • i18n
  • Add tests that demonstrate the downloads update properly when parent or child work changes in visibility.

Testing Instructions

In addition to the steps on Jira, please make sure the downloaded work notes do not break anonymity. (I've verified on development: PDF, EPUB, HTML.)

References

#3911

Credit

james_

weeklies (she/her)

@weeklies
Copy link
Contributor Author

weeklies commented Aug 15, 2023

I am not sure how to resolve the i18n-task failures, because when testing, the downloaded files required the keys to be downloads.download_preface.html.inspired_by.unrevealed rather than downloads.download_preface.inspired_by.unrevealed.

 i18n-tasks unused -l en 
+--------+-----------------------------------------------------------+----------------------------------------------------+
| Locale | Key                                                       | Value                                              |
+--------+-----------------------------------------------------------+----------------------------------------------------+
|   en   | downloads.download_preface.html.external_related_work     | %{work_link} by %{creator_link}.                   |
|   en   | downloads.download_preface.html.inspired_by.revealed      | Inspired by %{work_link} by %{creator_link}.       |
|   en   | downloads.download_preface.html.inspired_by.unrevealed    | Inspired by a work in an unrevealed collection.    |
|   en   | downloads.download_preface.html.translation_of.revealed   | Translation of %{work_link} by %{creator_link}.    |
|   en   | downloads.download_preface.html.translation_of.unrevealed | Translation of a work in an unrevealed collection. |
+--------+-----------------------------------------------------------+----------------------------------------------------+

@sarken
Copy link
Collaborator

sarken commented Aug 15, 2023

Have you tried adding comments with i18n-tasks-use hints?

@weeklies weeklies marked this pull request as draft August 15, 2023 08:21
@weeklies weeklies marked this pull request as ready for review August 15, 2023 11:04
@weeklies
Copy link
Contributor Author

Current test failure is related to this, not this PR. It looks like it happens intermittently.

Copy link
Contributor

@tickinginstant tickinginstant left a comment

Choose a reason for hiding this comment

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

This still needs some callbacks to expire the caches on all children when a work changes from unrevealed to revealed or vice versa.

(Either that, or it needs some nginx changes to make sure that downloads are only ever cached for ... IDK, a day? Some short amount of time. Download generation is pretty slow, though, so it's probably better to go with the callback.)

Also, there may be a similar issue with details about unrevealed child related works being listed in the parent, once the parent work has approved the relationship. Should that be addressed as part of this issue, or as a separate issue?

app/views/downloads/_download_preface.html.erb Outdated Show resolved Hide resolved
@weeklies weeklies marked this pull request as draft August 16, 2023 00:54
@weeklies
Copy link
Contributor Author

weeklies commented Aug 16, 2023

Some notes and doubts:

  1. For the case of child translations in an unrevealed collection, I am going with not displaying them in the notes of parent works even if they are approved. Because I think this looks a bit awkward:
    Translation into %{language} available: A work in an unrevealed collection.

  2. There are some inconsistencies with how downloads and work notes are displayed, which results in more varrying locale keys than I had planned. Let me know if any of them can be condensed.

    • I am also wondering if showing [Restricted Work] by creator is necessary, considering the creator probably wants their work to be hidden from guests?
  3. For some reason, download afterwords do not display related work titles that are translations of it. I can change this if desirable.

@weeklies weeklies changed the title AO3-6058 Hide byline of unrevealed work in notes of child related work AO3-6058 Hide byline of unrevealed work in notes of related work Aug 16, 2023
config/locales/views/en.yml Outdated Show resolved Hide resolved
@weeklies weeklies marked this pull request as ready for review August 17, 2023 01:03
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.

Regarding the inconsistencies you noted, yeah, let's definitely address those!

I made a two-sheet spreadsheet comparing the current text (you can see it on this test work) to the new versions, and left comments pointing out some things that need to change. (You can ignore the highlighting; that was just to make it easier to show PAC and Support chairs which text had actually changed.)

app/views/downloads/_download_afterword.html.erb Outdated Show resolved Hide resolved
config/locales/views/en.yml Outdated Show resolved Hide resolved
@weeklies
Copy link
Contributor Author

Sorry for throwing off the commit history, fixing it seems to always make it worse. 🤦

@weeklies
Copy link
Contributor Author

weeklies commented Sep 15, 2023

I stared at the i18n strings for too long and now I'm wondering if the presence and absence of an ending period should be standardized. (Not very important but to cover all the bases)

@sarken
Copy link
Collaborator

sarken commented Sep 19, 2023

I've been thinking about that period because these aren't really complete sentences with subjects and verbs and so on -- we just needed something to separate the log in instructions from the rest.

In fact, we were going to ask you to remove it from places like Translation into %{language} available: %{work_link} by %{creator_link}. but then I overthought it at the last minute and took that request out. 😆 We definitely shouldn't be adding it to %{work_link} by %{creator_link} or A work in an unrevealed collection (in work_approved_children), though.

So what if we just left it out in all cases and put the prompt to log in in parentheses, e.g.:

  • [Restricted Work] by %{creator_link} (Log in to access.)
  • Inspired by [Restricted Work] by %{creator_link} (Log in to access.)

@neuroalien neuroalien merged commit 0b64933 into otwcode:master Oct 29, 2023
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.

6 participants