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

Move author identifiers to version-controlled YAML file #9769

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

fakegithub01
Copy link
Contributor

@fakegithub01 fakegithub01 commented Aug 18, 2024

Closes #9666

Refactor

Technical

  • Created a new YAML file: openlibrary/plugins/openlibrary/config/author/author.yml.
  • Updated _get_author_config() function to load author identifiers from the new author.yml file.

Testing

  • Verify that the application correctly loads author configurations from author.yml.
  • Test relevant parts of the application to ensure no disruption of existing functionality.

Stakeholders

@jimchamp @mekarpeles

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.

Thanks @fakegithub01! Will QA this branch after the requested changes have been made.

openlibrary/plugins/openlibrary/config/author/author.yml Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/config/author/author.yml Outdated Show resolved Hide resolved
openlibrary/plugins/upstream/utils.py Outdated Show resolved Hide resolved
@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Aug 21, 2024
Co-authored-by: jimchamp <28732543+jimchamp@users.noreply.github.com>
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Aug 21, 2024
fakegithub01 and others added 3 commits August 21, 2024 07:45
Co-authored-by: jimchamp <28732543+jimchamp@users.noreply.github.com>
Co-authored-by: jimchamp <28732543+jimchamp@users.noreply.github.com>
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.

You didn’t actually rename the author.yml file as you changed the open() call.

Freso added a commit to Freso/openlibrary that referenced this pull request Aug 26, 2024
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
@fakegithub01
Copy link
Contributor Author

@Freso @jimchamp I have renamed the author.yml file to identifiers.yml as reviewed for changes. Can you please review the PR?

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.

Seems fine to me. I have left a couple of comments about related documentation that is slightly off now, but I don’t know if Jim will want that changed or not. I just wanted to note the filename so once Jim is available to review this, he wouldn’t have to point that out, so you could get this PR done with sooner. :)

identifiers = [Storage(t.dict()) for t in thing.identifiers if 'name' in t]
else:
identifiers = {}
# Load the author config from the author.yml file in the author directory
Copy link
Contributor

Choose a reason for hiding this comment

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

We’re only loading identifiers of the author config here… but that’s also currently the entirety of the config. I don’t know if that’s something that needs changing or not.

@@ -1222,13 +1222,16 @@ def _get_author_config():

The results are cached on the first invocation.
Any changes to /config/author page require restarting the app.
Copy link
Contributor

Choose a reason for hiding this comment

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

The /config/author page isn’t getting loaded anymore, so restarting won’t do much… :)

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.

Thanks, @fakegithub01!
I've tested this locally by adding a new author identifier entry. Verified that the new ID is present in the identifiers drop-down in the author edit screen. Newly added IDs were also present on the author view page.

Will open a follow-up PR that updates the documentation for these functions, and the equivalent edition identifier configuration functions, which, I suspect, were not updated.

@jimchamp jimchamp merged commit 9c4db66 into internetarchive:master Aug 27, 2024
4 checks passed
Freso added a commit to Freso/openlibrary that referenced this pull request Aug 27, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move config/author.yml out of infogami, into version controlled file
3 participants