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

Associated Works for Zenodo Records #48

Merged
merged 83 commits into from
May 16, 2022

Conversation

tjacovich
Copy link
Contributor

Adds associated works to citation targets in database. Updates current citation targets when a new associated target is added. Adds maintenance task to find associated works for all entries. Includes associated works in forwarded messages to Master.

tjacovich and others added 30 commits March 22, 2022 10:34
…o 'versions' in parsed metadata to handle it returning as an array.
…Modified forward._build_nonbib_record to accept version records, and place bibcodes in the 'ASSOCIATED' data_link_row.
…bibcode to db_version_bibcodes as this bibcode will not be found in the public schema until it has finished being processed. generated initial skeleton for task_process_updated_associated_works
…ion from database and forward changes to master without calling the webhook.
… only added to new citations if other versions already exist in the db. Refactoring to make updating targets in the db easier.
…tuation where all_verions_doi is undefined due to doi not being defined, or parsed_metadata failing
… call to db.updated_citation_target_metadata() to task_process_update_associated_works().
…tadata check in task.py. Fixed error in column add for alembic version associated_works.
…ified _build_nonbib_record to properly include associate works. Fixed checks on versions to match expected output.
@tjacovich tjacovich marked this pull request as ready for review March 24, 2022 22:38
Copy link
Contributor

@marblestation marblestation 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! I have some suggestions, but I believe it is basically ready to be merged

@@ -36,7 +36,7 @@ def store_event(app, data):
stored = True
return stored

def store_citation_target(app, citation_change, content_type, raw_metadata, parsed_metadata, status):
def store_citation_target(app, citation_change, content_type, raw_metadata, parsed_metadata, status, associated=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's homogenize spaces before/after equal signs (choose the same style as Roman's cookie cutter tool)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Homogenizing around no space when in function def or calls, but space when assigning to variable.

ADSCitationCapture/db.py Outdated Show resolved Hide resolved
ADSCitationCapture/doi.py Outdated Show resolved Hide resolved
ADSCitationCapture/doi.py Outdated Show resolved Hide resolved
ADSCitationCapture/doi.py Outdated Show resolved Hide resolved
ADSCitationCapture/tests/test_tasks.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ADSCitationCapture/tasks.py Show resolved Hide resolved
)
#update associated works for all versions in db
logger.info('Calling task process_updated_associated_works')
task_process_updated_associated_works.delay(associated_citation_change, associated_version_bibcodes)
Copy link
Contributor

Choose a reason for hiding this comment

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

The function _collect_associated_works is calling task_process_updated_associated_works, it feels to me like not a specific simple function that does one thing (i.e., return some values) but two things (i.e., return some values and trigger further processing). I would split it in two, one gets the info and the other triggers the processing. That should make the code more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Broke these into two functions and moved the call for the processing to afters the citation target is stored so that way the new target is guaranteed to be in the database.

@tjacovich
Copy link
Contributor Author

Formatting updates are largely complete, but need to wait to rebase on patched manual curation

@marblestation marblestation merged commit b29e321 into adsabs:master May 16, 2022
tjacovich added a commit to tjacovich/ADSCitationCapture that referenced this pull request May 16, 2022
@tjacovich tjacovich linked an issue Jul 12, 2022 that may be closed by this pull request
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.

Expand unit tests to cover forwarded messages
2 participants