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

Displays author name on merge #9920

Conversation

DanielleInkster
Copy link
Contributor

@DanielleInkster DanielleInkster commented Oct 1, 2024

Closes #9811

Displaying the author’s name next to the OL...A identifier making it easier to distinguish between works by different authors.

Technical

utils.js
Added get_author_name function to fetch the name of the author associated with the id and append to the record.

AuthorRoleTable.vue
Added p tag to display authors name, as well as class to formate with CSS.

MergeTable.vue
Created enhancedRecords to include author name and added to template in order to display. Added CSS to display name next to ID.

*** I've not added enhancedRecords to the merge function in this file as I couldn't verify if this would impact what is saved when merging works. Can someone please clarify?***

Testing

  1. Run command npm run serve --component=MergeUI.
  2. Open merge view (http://localhost:8080/static/components/?records=OL1233242W,OL12312W)
  3. Verify merge view now displays the author names next to the ids
  4. Verify merging works as expected.

*** I was not able to find any JS tests for the files associated with this ticket. If needed, I can create if directed to the correct directory.***

Screenshot

Screenshot 2024-10-01 at 17 23 46

Stakeholders

@cdrini

@cdrini cdrini added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Oct 2, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Awesome @DanielleInkster ! A few small fixes but looking great so far 😊 Note I noticed this in my testing erroring:

https://testing.openlibrary.org/works/merge?records=OL6151392W,OL31613496W

Some sort of error is happening and only the result row is getting rendered!

openlibrary/components/MergeUI/AuthorRoleTable.vue Outdated Show resolved Hide resolved
openlibrary/components/MergeUI/MergeTable.vue Outdated Show resolved Hide resolved
openlibrary/components/MergeUI/utils.js Outdated Show resolved Hide resolved
openlibrary/components/MergeUI/utils.js Outdated Show resolved Hide resolved
openlibrary/components/MergeUI/MergeTable.vue Outdated Show resolved Hide resolved
@cdrini
Copy link
Collaborator

cdrini commented Oct 2, 2024

*** I've not added enhancedRecords to the merge function in this file as I couldn't verify if this would impact what is saved when merging works. Can someone please clarify?***

Yes that's exactly correct! We don't want to use enhanced_records during merging since then it'll try to save the author names in a weird place :P

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.12%. Comparing base (ce16a79) to head (48a7178).
Report is 484 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9920      +/-   ##
==========================================
+ Coverage   16.06%   17.12%   +1.06%     
==========================================
  Files          90       89       -1     
  Lines        4769     4752      -17     
  Branches      832      831       -1     
==========================================
+ Hits          766      814      +48     
+ Misses       3480     3428      -52     
+ Partials      523      510      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cdrini cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Oct 2, 2024
@DanielleInkster DanielleInkster force-pushed the 9811/feature/display-author-name-on-merge branch from 2253ae3 to 9474056 Compare October 5, 2024 13:18
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Oct 5, 2024
@DanielleInkster
Copy link
Contributor Author

https://testing.openlibrary.org/works/merge?records=OL6151392W,OL31613496W

@cdrini - I'm not able to access this unfortunately.
Screenshot 2024-10-05 at 14 22 13

Copy link
Contributor

@Freso Freso left a comment

Choose a reason for hiding this comment

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

Idk Vue and my JavaScript is very rusty, and I also haven’t really looked at the HTML structure of the rest of the site, so take this comment with a tiny grain of salt! 😅

openlibrary/components/MergeUI/AuthorRoleTable.vue Outdated Show resolved Hide resolved
@DanielleInkster DanielleInkster force-pushed the 9811/feature/display-author-name-on-merge branch from 9474056 to 48a7178 Compare October 12, 2024 15:17
@DanielleInkster
Copy link
Contributor Author

@cdrini - I think I've covered everything except the error - I'm not able to access the link you've included. Can you please take a look when you've got a minute?

@cdrini
Copy link
Collaborator

cdrini commented Oct 15, 2024

Hi @DanielleInkster apologies for the delay, I was on holiday last week! IA/OL are currently offline, but I'll be able to test this once they're back up (see #9947 (comment) ).

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

This looks great @DanielleInkster , nice work! One small style tweak. I tested merging works and the author names loaded/displayed perfectly!

I noticed one issue, which was that the author name didn't appear for the "merged" work, but we can tackle that in a future issue/pr.

image

openlibrary/components/MergeUI/MergeTable.vue Outdated Show resolved Hide resolved
@cdrini cdrini merged commit 58bd787 into internetarchive:master Dec 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When merging works, display author names
4 participants