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

Read Work identifiers from file #9798

Conversation

Freso
Copy link
Contributor

@Freso Freso commented Aug 26, 2024

Closes #9234

feature/fix/refactor

Technical

This is taking the same steps applied to editions[1] and authors[2] and applying it to works. get_work_config() is currently not used for anything, probably because we’re still waiting for an actual interface to edit the Work identifiers, so this is more of a preliminary step to ensure that Works will be handled in the same way that Editions and Authors are. (And to allow for PRs to add Work identifiers while waiting for that UI, rather than limiting that to OL admins.)

[1] #9234
#9483
[2] #9666
#9769

(I asked 3 weeks ago whether a PR for this would be welcome or not, and never got a response, so now I just made the PR.)

Testing

Only the pre-commit hooks were done. The mypy hook failed, but Idk if it’s a local issue that /infogami isn’t found or not.

mypy.....................................................................Failed
- hook id: mypy
- duration: 0.37s
- exit code: 2

mypy: can't read file '[…]/openlibrary/infogami': No such file or directory

Stakeholders

@jimchamp

This is taking the same steps applied to editions[1] and authors[2] and
applying it to works. `get_work_config()` is currently not used for
anything, probably because we’re still waiting for an actual interface
to edit the Work identifiers[3], so this is more a preliminary step to
ensure that Works will be handled in the same way that Editions and
Authors are. (And to allow for PRs to add Work identifiers while waiting
for that UI.)

[1] internetarchive#9234
    internetarchive#9483
[2] internetarchive#9666
    internetarchive#9769
[3] internetarchive#3430
@jimchamp jimchamp added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label Sep 3, 2024
Copy link
Collaborator

@jimchamp jimchamp 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 good, but I can't merge it in until there's some way to add identifiers to works.

Marking this as State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] until #3430 is resolved.

@Freso
Copy link
Contributor Author

Freso commented Sep 4, 2024

@jimchamp Alright! So… should I pester OL admins to get Work IDs added, or should I append them to this PR, or should I make separate PRs branching off of the code here, or… ? It seems like otherwise they just get left behind/forgotten about—e.g., the Work ID from #9814 has not been added to https://openlibrary.org/config/work.yml AFAICT.

@jimchamp
Copy link
Collaborator

jimchamp commented Sep 5, 2024

The work and author IDs added by #9814 have not yet been deployed to production, but are available in our staging environment.
image

Please don't make any new PRs from this branch. When you need a new work identifier added to the Infogami work config, I can get it approved and add it for you. I just ask that you do the following:

  1. Create a new issue for adding the ID. This will help us audit these identifiers in the future.
  2. Mention me in the issue to get my attention. If I'm taking too long to add the identifier, just message me in Slack.

I'm not sure what the status is for #3430, but it is linked to a PR (#8346) that adds a work identifier section to the edit work form. It looks like it has been abandoned, but if it were ever merged in it would unblock this PR. I'm not sure if it just needs a rebase, or if more substantial changes are needed. @cdrini would know more.

@Freso
Copy link
Contributor Author

Freso commented Sep 6, 2024

@jimchamp https://staging.openlibrary.org/config/work.yml doesn’t have the Work level identifier for #9814 either. Are you sure it was added? The screenshot seems to be Edition identifiers (there are no Amazon or YouTube Work level identifiers to my knowledge, anyway).

@jimchamp
Copy link
Collaborator

jimchamp commented Sep 7, 2024

I'm sure that they are there. The /config/*.yml files are no longer the source of truth for author and edition identifiers. We read these from the checked in file, but never write those identifiers to the Infogami record.

The screenshot was from an author edit page.

I just added the SNU(/ICCU) identifier to the /config/work.yml. I think that this was overlooked when the PR that added this identifier to the version controlled files was merged.

@Freso
Copy link
Contributor Author

Freso commented Sep 7, 2024

The /config/*.yml files are no longer the source of truth for author and edition identifiers. We read these from the checked in file, but never write those identifiers to the Infogami record.

I know. The whole point of this PR is to have Works act the same as the other two. 😉 Are you confusing Edition/Author identifiers with Work ones? I’m trying to be very explicit which ones I’m talking about, esp. since they’re (currently) handled in different ways. Until this PR is merged, Work identifiers are still in Infogami.

I just added the SNU(/ICCU) identifier to the /config/work.yml. I think that this was overlooked when the PR that added this identifier to the version controlled files was merged.

Thank you! I’ll update this PR later to reflect this.


Somewhat relatedly, would this PR be acceptable if it was just the YAML file and not the get_work_config() functions? I could submit a change request to #8346 (either a PR upstream, or just a review comment with the changes directly on GH) to deal with the required changes for pulling the identifiers from the file.

@jimchamp
Copy link
Collaborator

jimchamp commented Sep 9, 2024

Why would you want to merge in only the identifiers.yml file? What problem does this solve?

To be perfectly clear, I'm not merging in any dead code or configuration files that are not used by the application.

@jimchamp jimchamp changed the base branch from master to version-controlled-work-identifiers September 10, 2024 17:05
@jimchamp
Copy link
Collaborator

A decision was made during today's community call to merge these changes into a new branch, which will later be merged when we are able to add work IDs via the UI.

The version-controlled-work-identifiers branch has been created for these work IDs. Please use this as the base branch when adding new work identifiers.

@jimchamp jimchamp merged commit b025e1b into internetarchive:version-controlled-work-identifiers Sep 10, 2024
4 checks passed
@Freso Freso deleted the version-control-work-ids branch September 10, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Open Library to use checked in source for adding New Identifiers
2 participants