diff --git a/hepdata/modules/records/api.py b/hepdata/modules/records/api.py index c53fb338..04b9b495 100644 --- a/hepdata/modules/records/api.py +++ b/hepdata/modules/records/api.py @@ -315,16 +315,19 @@ def should_send_json_ld(request): def get_commit_message(ctx, recid): """ Returns a commit message for the current version if present. + Will return the highest ID of a version-recid pairing. :param ctx: :param recid: """ try: + # Select the most recent commit (greatest ID) commit_message_query = RecordVersionCommitMessage.query \ - .filter_by(version=ctx["version"], recid=recid) + .filter_by(version=ctx["version"], recid=recid) \ + .order_by(RecordVersionCommitMessage.id.desc()) if commit_message_query.count() > 0: - commit_message = commit_message_query.one() + commit_message = commit_message_query.first() ctx["revision_message"] = { 'version': commit_message.version, 'message': commit_message.message} diff --git a/hepdata/modules/records/utils/submission.py b/hepdata/modules/records/utils/submission.py index 0cb52b4e..3fd1eafc 100644 --- a/hepdata/modules/records/utils/submission.py +++ b/hepdata/modules/records/utils/submission.py @@ -715,6 +715,7 @@ def do_finalise(recid, publication_record=None, force_finalise=False, print('Finalising record {}'.format(recid)) hep_submission = get_latest_hepsubmission(publication_recid=recid) + error_message = None generated_record_ids = [] if hep_submission \ @@ -809,14 +810,26 @@ def do_finalise(recid, publication_record=None, force_finalise=False, site_url = current_app.config.get('SITE_URL', 'https://www.hepdata.net') tweet(record.get('title'), record.get('collaborations'), site_url + '/record/ins{0}'.format(record.get('inspire_id')), version) - return json.dumps({"success": True, "recid": recid, "data_count": len(submissions), "generated_records": generated_record_ids}) - except NoResultFound: - print('No record found to update. Which is super strange.') - + error_message = 'No record found to update. Which is super strange.' + + # If we have not returned, then we set an error message + if error_message is None: + error_message = "An error occurred, please try again" + + # If we get to here, we have not returned + # Do some cleanup (rollback and return error message) + if error_message: + print(error_message) + db.session.rollback() + return json.dumps({ + "success": False, + "recid": recid, + "errors": [error_message] + }) else: return json.dumps( {"success": False, "recid": recid, diff --git a/tests/conftest.py b/tests/conftest.py index 23831e35..fae45710 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -187,10 +187,11 @@ def create_blank_test_record(): return submission -def create_test_record(file_location): +def create_test_record(file_location, overall_status='finished'): """ Helper function to create a dummy record with data. :param file_location: Path to the data directory. + :param overall_status: Allows setting of custom overall status. Defaults to 'finished'. :returns test_submission: The newly created submission object """ record = {'title': 'HEPData Testing', @@ -201,7 +202,7 @@ def create_test_record(file_location): # Set up a new test submission test_submission = process_submission_payload(**record) # Ensure the status is set to `finished` so the related data can be accessed. - test_submission.overall_status = 'finished' + test_submission.overall_status = overall_status record_dir = get_data_path_for_record(test_submission.publication_recid, str(int(round(time.time())))) shutil.copytree(file_location, record_dir) process_submission_directory(record_dir, os.path.join(record_dir, 'submission.yaml'), diff --git a/tests/records_test.py b/tests/records_test.py index 4646020c..22c10fea 100644 --- a/tests/records_test.py +++ b/tests/records_test.py @@ -36,6 +36,7 @@ from flask_login import login_user from invenio_accounts.models import User from invenio_db import db +from sqlalchemy.exc import MultipleResultsFound import pytest from werkzeug.datastructures import FileStorage import requests_mock @@ -45,8 +46,8 @@ move_files, get_all_ids, has_upload_permissions, \ has_coordinator_permissions, create_new_version, \ get_resource_mimetype, create_breadcrumb_text, format_submission, \ - format_resource, get_related_to_this_hepsubmissions, get_related_hepsubmissions, get_related_datasubmissions, \ - get_related_to_this_datasubmissions + format_resource, get_commit_message, get_related_to_this_hepsubmissions, \ + get_related_hepsubmissions, get_related_datasubmissions, get_related_to_this_datasubmissions from hepdata.modules.records.importer.api import import_records from hepdata.modules.records.utils.analyses import update_analyses from hepdata.modules.records.utils.submission import get_or_create_hepsubmission, process_submission_directory, \ @@ -59,7 +60,7 @@ from hepdata.modules.records.utils.workflow import update_record, create_record from hepdata.modules.records.views import set_data_review_status from hepdata.modules.submission.models import HEPSubmission, DataReview, \ - DataSubmission, DataResource, License, RelatedRecid, RelatedTable + DataSubmission, DataResource, License, RecordVersionCommitMessage, RelatedRecid, RelatedTable from hepdata.modules.submission.views import process_submission_payload from hepdata.modules.submission.api import get_latest_hepsubmission from tests.conftest import TEST_EMAIL @@ -1120,6 +1121,52 @@ def test_generate_license_data_by_id(app): } +def test_get_commit_message(app): + """ + Tests functionality of the get_commit_message function. + Ensures no instances of None, that duplicate commit messages + are handled correctly, and only the most recent + RecordVersionCommitMessage is returned. + """ + # We want to ensure duplicate entries + test_version, test_recid = 1, 1 + # How many records we want to insert + insert_amount = 5 + + # First we check no insertion, then we check insertion + for should_insert in [False, True]: + # Only insert on the second go + if should_insert: + # Insert a bunch of duplicate entries + for i in range(0, insert_amount): + new_record = RecordVersionCommitMessage( + recid=test_recid, + version=test_version, + # Setting message to a unique value + message=str(insert_amount) + ) + db.session.add(new_record) + db.session.commit() + + # Result of get_commit_message is added to ctx as revision_message + ctx = {"version": test_version} + + # We always want to check that duplicates are not returned + try: + get_commit_message(ctx, test_recid) + except MultipleResultsFound as e: + raise AssertionError(e) + + # revision_message only exists if we should insert + assert ("revision_message" in ctx) == should_insert + + # Check the most recent has been retrieved + if should_insert: + # We know it's the most recent as message + # is set to the highest inserted range, equal to insert_amount. + ctx["revision_message"]["message"] = str(insert_amount) + + def test_version_related_functions(app): """ Attempts to bulk test the related functions for both data tables and submissions (records/api): diff --git a/tests/submission_test.py b/tests/submission_test.py index a1ac2ba0..f15f638b 100644 --- a/tests/submission_test.py +++ b/tests/submission_test.py @@ -22,11 +22,15 @@ # waive the privileges and immunities granted to it by virtue of its status # as an Intergovernmental Organization or submit itself to any jurisdiction. +import json import logging import os import shutil import time +from datetime import datetime from time import sleep +from unittest.mock import patch, MagicMock +from sqlalchemy.exc import NoResultFound from invenio_db import db import pytest @@ -42,15 +46,15 @@ get_related_to_this_datasubmissions, get_related_to_this_hepsubmissions, get_table_data_list, - process_saved_file + process_saved_file, get_commit_message ) from hepdata.modules.records.utils.common import infer_file_type, contains_accepted_url, allowed_file, record_exists, \ - get_record_contents, is_histfactory + get_record_contents, is_histfactory, get_record_by_id from hepdata.modules.records.utils.data_files import get_data_path_for_record from hepdata.modules.records.utils.submission import process_submission_directory, do_finalise, unload_submission, \ cleanup_data_related_recid from hepdata.modules.submission.api import get_latest_hepsubmission, get_submission_participants_for_record -from hepdata.modules.submission.models import DataSubmission, HEPSubmission, RelatedRecid +from hepdata.modules.submission.models import DataSubmission, HEPSubmission, RelatedRecid, RecordVersionCommitMessage from hepdata.modules.submission.views import process_submission_payload from hepdata.config import HEPDATA_DOI_PREFIX from tests.conftest import create_test_record @@ -713,3 +717,98 @@ def test_status_reset_error(app, mocker, caplog): assert(caplog.records[0].levelname == "ERROR") assert(caplog.records[0].msg == "Exception while cleaning up: Could not clean up the submission") + + +# Patching to force no-op on function: process_last_updates() +@patch("hepdata.ext.opensearch.document_enhancers.process_last_updates") +def test_do_finalise_commit_message(app, admin_idx): + """ + Tests the do_finalise function. + + Here we are testing the commit message functionality, ensuring proper rollback in the event of an error. + """ + + # Insert the testing record data + with app.app_context(): + admin_idx.recreate_index() + # Create test submission/record + hepdata_submission = create_test_record( + os.path.abspath('tests/test_data/test_submission'), + overall_status='todo' + ) + + # We're going to add an extra commit message + conflicting_record = RecordVersionCommitMessage( + recid=hepdata_submission.publication_recid, + version=hepdata_submission.version, + message="OldMessage" + ) + + db.session.add(conflicting_record) + db.session.commit() + + # Create record data and prepare + record = get_record_by_id(hepdata_submission.publication_recid) + record["creation_date"] = str(datetime.today().strftime('%Y-%m-%d')) + + # Now we mock the get_record_by_id function to cause an error. + with patch("hepdata.modules.records.utils.submission.get_record_by_id") as mock_get_record: + class MockRecord(MagicMock, dict): + # Mocking the invenio-records Record class + # Used to create a mock also of type dict + pass + + # Create mock record object, set its value + # and set it to cause an error. + mock_record = MockRecord() + mock_get_record.return_value = mock_record + # Set the record to raise exception when commit() is called + mock_record.commit.side_effect = NoResultFound() + # Injecting specific dictionary keys to avoid error. + mock_record.__getitem__.side_effect = lambda key: 1 if key == 'item_doi' else None + + # Now we run the do_finalise function (PATCHED) + result = do_finalise( + hepdata_submission.publication_recid, + publication_record=record, + commit_message="NewMessage", + force_finalise=True, + convert=False + ) + # Convert str(json)->dict + result = json.loads(result) + + # Get all commit messages + commit_messages = RecordVersionCommitMessage.query.filter_by( + recid=hepdata_submission.publication_recid + ).all() + + # Ensure that no new messages have inserted + assert len(commit_messages) == 1 + assert commit_messages[0].message == "OldMessage" + + # Confirm that the result response exists and is correct + assert "errors" in result + assert len(result["errors"]) == 1 + assert result["errors"][0] == "No record found to update. Which is super strange." + + # Run do_finalise again, but UNPATCHED + result = do_finalise( + hepdata_submission.publication_recid, + publication_record=record, + commit_message="NewMessage", + force_finalise=True, + convert=False + ) + # Convert str(json)->dict + result = json.loads(result) + + # Get the commit messages + commit_messages = RecordVersionCommitMessage.query.filter_by( + recid=hepdata_submission.publication_recid + ).all() + + # `NewMessage` should have inserted, and no error found. + assert len(commit_messages) == 2 + assert commit_messages[-1].message == "NewMessage" + assert "errors" not in result