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-5649 Sort authors_to_sort_on to match byline #3555

Merged
merged 5 commits into from Sep 12, 2019

Conversation

redsummernight
Copy link
Member

Issue

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

Purpose

When a work has multiple co-creators, authors_to_sort_on should sort them similar to how the byline does it:

def byline_text(creation, only_path, text_only = false)
if creation.respond_to?(:author)
creation.author
else
pseuds = []
pseuds << creation.authors if creation.authors
pseuds << creation.pseuds if creation.pseuds && (!@preview_mode || creation.authors.blank?)
pseuds = pseuds.flatten.uniq.sort

Testing Instructions

How can the Archive's QA team verify that this is working as you intended? (If you have access, please copy this into the JIRA ticket for them!)

References

See issue. Works need to be reindexed.

@tickinginstant
Copy link
Contributor

Capitalization interacts with this in interesting ways. Pseuds have a custom comparison function that downcases:

def <=>(other)
(self.name.downcase <=> other.name.downcase) == 0 ? (self.user_name.downcase <=> other.user_name.downcase) : (self.name.downcase <=> other.name.downcase)
end

So the byline helper, which sorts the pseuds before retrieving each pseud's byline (and therefore uses this comparison function), should place "alice" before "Zeke," because when they're downcased, "alice" < "zeke". But this PR sorts after retrieving the names but before downcasing, and "alice" > "Zeke", so I believe this would sort a work co-created by "alice" and "Zeke" among the creators starting with Z.

I tried to run a test to verify, and I think this would be the order for these ten bylines:

  1. by alice
  2. by alice, zach
  3. by Anna
  4. by alice, Anna
  5. by Anna, zach
  6. by Anna, Zeke
  7. by zach
  8. by Zeke
  9. by alice, Zeke
  10. by zach, Zeke

@redsummernight
Copy link
Member Author

Thanks! I moved the sort ahead of getting the names.

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.

Looks good! I redid the 10 bylines test with the new code, and it looks nicely ordered.

@sarken sarken added the Deferred or Superseded Should not be merged because it is waiting on more thought, another pull request, etc label May 25, 2019
@sarken
Copy link
Member

sarken commented May 25, 2019

Marked deferred because this is going in with the Warning class rename because they both need reindexing.

@sarken sarken added the Needs Reindex Contains changes that will require Elasticsearch reindexing label Jun 29, 2019
@sarken sarken removed the Deferred or Superseded Should not be merged because it is waiting on more thought, another pull request, etc label Sep 12, 2019
@sarken sarken merged commit dfdb367 into otwcode:master Sep 12, 2019
@redsummernight redsummernight deleted the AO3-5649-author-sort branch September 12, 2019 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Reindex Contains changes that will require Elasticsearch reindexing Reviewed: Ready to Merge
Projects
None yet
3 participants