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

Theoretical records #683

Merged
merged 59 commits into from
Aug 18, 2023
Merged

Theoretical records #683

merged 59 commits into from
Aug 18, 2023

Conversation

ItIsJordan
Copy link
Collaborator

@ItIsJordan ItIsJordan commented Aug 3, 2023

This PR closes #58 and closes #323.

This command executes the data migration SQL found in the scripts/hepdata_related_update.sql file:

psql -U hepdata -h localhost -d hepdata -f scripts/hepdata_related_update.sql

Adds the RelatedTable class to the submission model containing entries for related table DOIs.
Adds the new related_tables variable to the record view.
Updates the process_data_file function to handle the new related_table DOI entry for new submission objects.
Updates table_details.html to display related doi list div.
The previous related_tables code was setting its value in the table_contents data to be a collection of RelatedTable objects. It is now a list of strings containing doi entries.
Adds the data from related_tables into the generate_table_structure function to be properly returned.
Updates ajax used to render HEPData tables to include related table doi rendering.
I misunderstood that the data would be accessed from the context. Updated HTML to reflect this.
Adds the related recid functionality to the models.py file in submission. Modifies the HEPSubmission class.
Updates records api to add related recids variable to the context for rendering.
Add functionality to move related recid data from the collected submission info document variable into the database.
Includes html tags for recids relation. Adds related recid section to the left side panel in the record page. Also adds placeholder for second section.
Adds a function to retrieve all records in the RelatedRecId table where the related object recid is the same.
Adds functionality for recid relation. Adds support for js/html rendering of other relating records.
Updates code to ensure the value saved under RelatedRecId->this_recid to be the recid as an integer. Changed from using a doi string.
Updates the RelatedRecId class in the database model to use Integer instead of String(128). This enforces reference by HEPData ID, and not inspire ID.
Adds functionality hide the related recids, and related table doi object elements when empty.
Updates the record found text to be more accurate.
Implements the query to get other DataSubmission objects relating to the self object. Fixed placeholder empty list for this function too.
Fixed an issue where the previous sandbox filtering logic was incorrect. Now fixed and updated comments.
Fix now broken test_data_processing test as new related data lists were not set.
Renames the functions in HEPSubmission and DataSubmission used for retrieving other related HS/DS objects. Both functions now properly filter by checking the relevant HEPSubmission object's overall_status value. Both functions also now return the objects themselves.
Adds a function to test the related record ID and table DOI submission and linking functionality.

Adds 3 test submissions based on test_submission, but with entries for related record ID and table DOIs in Submission.yaml
Updates the hepdata-validator package in requirements.txt to use the new git branch.
Updates the test_related_records function and its test data.

- Removes useless data in the testing data, now down to a skeleton submission.

- Moves tests to a new directory for cleanliness, add 4th test to check for invalid data DOI.
Removes a blank line from views.py that is not needed.
Adds e2e test to test record page display of related record ID and data DOI values. This commit has completed record ID testing.
Adds a comment explaining that related record entries are cleared in cleanup_submission and why.
Fixes a bug created in f59a60f where removal of data resource lines caused additional resources to only be deleted if new additional resource data is deleted. This cleanup also needs to happen outside of the later if statement.
Add the SQL required to update an existing database to the updated schema version.
@coveralls
Copy link

coveralls commented Aug 3, 2023

Coverage Status

coverage: 82.712% (+0.2%) from 82.54% when pulling 4afa094 on theoretical-records into c1dfaf1 on main.

Improves html tags and updates tests to reflect changes.
Adds overflow to html the related list.
Adds a removed break tag back into publication_record.html
Removes spaces on array entries in the related test yaml files. Also removes a number of blank tables where "---" was at the end of some testing files.
Improves clarity of the titles associated with related recid/doi items on the records page html.
Improves the header text language and styling of the record page related recid/doi sections.
Update the records page, improving the text size and language on related data titles.
@sjmf sjmf self-assigned this Aug 11, 2023
Copy link
Collaborator

@sjmf sjmf left a comment

Choose a reason for hiding this comment

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

Some small bits and bobs to do, but looks 99.99% good to me!

hepdata/modules/submission/models.py Show resolved Hide resolved
hepdata/modules/submission/models.py Show resolved Hide resolved
hepdata/modules/submission/models.py Show resolved Hide resolved
scripts/hepdata_related_update.sql Outdated Show resolved Hide resolved
tests/e2e/test_records.py Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
hepdata/modules/theme/assets/scss/record.scss Show resolved Hide resolved
hepdata/modules/records/views.py Show resolved Hide resolved
hepdata/modules/records/utils/submission.py Outdated Show resolved Hide resolved
Copy link
Member

@GraemeWatt GraemeWatt 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, @ItIsJordan! Thanks for all your hard work on this. I made a few comments that are hopefully fairly easy to address. Please also check Samantha's comments from last week: there were a few that I've left unresolved as they don't seem to be addressed in 6af6ee2 (you might need to log in to GitHub to see all the comments). Please also merge the latest updates from the main branch.

ItIsJordan and others added 5 commits August 18, 2023 12:50
Update the hepdata-validator package in requirements.txt to use the updated version (0.3.4), and not the git branch.
Updates code where accessing submission document as the schema has now renamed "related_to_hepdata_recids" to "related_to_hepdata_records".
Updates commenting/code readability for related data testing, models and JS.
Updates the related record html to now be contained within a component template (related_recids.html). Updates publication_record.html to use this. Also updates related_record.html to include related record data.

Adds a conditional break tag to fix formatting when related record data does not exist.
* Need to add related_recids and related_to_this_recids to ctx.
Copy link
Member

@GraemeWatt GraemeWatt left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments, @ItIsJordan. In testing I found that pages displayed using the related_record.html template were missing expected links to related records. This was because the ctx is set differently from publication_record.html, so the related_recids and related_to_this_recids variables need to be added separately. I've pushed a fix in commit 4afa094. I'll merge now and deploy next week.

@GraemeWatt GraemeWatt merged commit 18881a4 into main Aug 18, 2023
17 checks passed
@GraemeWatt GraemeWatt deleted the theoretical-records branch August 18, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants