Merge pull request #1299 from microbiomedata/issue-field-notes-90-explicit-submission-lock-endpoint

Add endpoint for explicitly requesting `SubmissionMetadata` lock

Author: pkalita-lbl

nmdc_server/api.py

@@ -649,10 +649,10 @@ async def get_submission(
     db: Session = Depends(get_db),
     user: models.User = Depends(get_current_user),
 ):
-    submission = db.query(SubmissionMetadata).get(id)
+    submission: Optional[models.SubmissionMetadata] = db.query(SubmissionMetadata).get(id)
     if submission is None:
         raise HTTPException(status_code=404, detail="Submission not found")
-    _ = crud.try_get_submission_lock(db, submission.id, user.id)
+
     if user.is_admin or crud.can_read_submission(db, id, user.orcid):
         permission_level = None
         if user.is_admin or user.orcid in submission.owners:
@@ -663,16 +663,19 @@ async def get_submission(
             permission_level = models.SubmissionEditorRole.metadata_contributor.value
         elif user.orcid in submission.viewers:
             permission_level = models.SubmissionEditorRole.viewer.value
-        return schemas_submission.SubmissionMetadataSchema(
+        submission_metadata_schema = schemas_submission.SubmissionMetadataSchema(
             status=submission.status,
             id=submission.id,
             metadata_submission=submission.metadata_submission,
             author_orcid=submission.author_orcid,
             created=submission.created,
             author=schemas.User(**submission.author.__dict__),
-            locked_by=schemas.User(**submission.locked_by.__dict__),
             permission_level=permission_level,
         )
+        if submission.locked_by is not None:
+            submission_metadata_schema.locked_by = schemas.User(**submission.locked_by.__dict__)
+        return submission_metadata_schema
+
     raise HTTPException(status_code=403, detail="Must have access.")
 
 
@@ -862,22 +865,63 @@ async def delete_submission(
     return Response(status_code=status.HTTP_204_NO_CONTENT)
 
 
+@router.put("/metadata_submission/{id}/lock")
+async def lock_submission(
+    response: Response,
+    id: str,
+    db: Session = Depends(get_db),
+    user: models.User = Depends(get_current_user),
+) -> schemas.LockOperationResult:
+    submission: Optional[SubmissionMetadata] = db.query(SubmissionMetadata).get(id)
+    if not submission:
+        raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Submission not found")
+
+    # Attempt to acquire the lock
+    lock_acquired = crud.try_get_submission_lock(db, submission.id, user.id)
+    if lock_acquired:
+        return schemas.LockOperationResult(
+            success=True,
+            message=f"Lock successfully acquired for submission with ID {id}.",
+            locked_by=submission.locked_by,
+            lock_updated=submission.lock_updated,
+        )
+    else:
+        response.status_code = status.HTTP_409_CONFLICT
+        return schemas.LockOperationResult(
+            success=False,
+            message="This submission is currently locked by a different user.",
+            locked_by=submission.locked_by,
+            lock_updated=submission.lock_updated,
+        )
+
+
 @router.put("/metadata_submission/{id}/unlock")
 async def unlock_submission(
-    id: str, db: Session = Depends(get_db), user: models.User = Depends(get_current_user)
-) -> str:
+    response: Response,
+    id: str,
+    db: Session = Depends(get_db),
+    user: models.User = Depends(get_current_user),
+) -> schemas.LockOperationResult:
     submission = db.query(SubmissionMetadata).get(id)
     if not submission:
-        raise HTTPException(status_code=404, detail="Submission not found")
+        raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Submission not found")
+
     # Then verify session user has the lock
     has_lock = crud.try_get_submission_lock(db, submission.id, user.id)
     if not has_lock:
-        raise HTTPException(
-            status_code=400, detail="This submission is currently being edited by a different user."
+        response.status_code = status.HTTP_409_CONFLICT
+        return schemas.LockOperationResult(
+            success=False,
+            message="This submission is currently locked by a different user.",
+            locked_by=submission.locked_by,
+            lock_updated=submission.lock_updated,
         )
     else:
         crud.release_submission_lock(db, submission.id)
-        return f"Submission lock released successfully for submission with ID {id}"
+        return schemas.LockOperationResult(
+            success=True,
+            message=f"Submission lock released successfully for submission with ID {id}",
+        )
 
 
 @router.post(

nmdc_server/crud.py

@@ -497,10 +497,17 @@ def try_get_submission_lock(db: Session, submission_id: str, user_id: str) -> bo
     submission_record = db.query(models.SubmissionMetadata).get(submission_id)
     if not submission_record:
         raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Submission not found")
-    user_record = db.query(models.User).get(user_id)
+    user_record: Optional[models.User] = db.query(models.User).get(user_id)
     if not user_record:
         raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="User not found")
 
+    # Check that the user has sufficient permissions to obtain the lock
+    if not (user_record.is_admin or can_read_submission(db, submission_id, user_record.orcid)):
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN,
+            detail="User does not have permission to obtain lock",
+        )
+
     current_lock_holder = submission_record.locked_by
     if not current_lock_holder or current_lock_holder.id == user_id:
         # Either the given user already has the lock, or no one does

nmdc_server/schemas.py

@@ -658,3 +658,10 @@ class User(BaseModel):
 
     class Config:
         orm_mode = True
+
+
+class LockOperationResult(BaseModel):
+    success: bool
+    message: str
+    locked_by: Optional[User]
+    lock_updated: Optional[datetime]

tests/test_submission.py

@@ -27,6 +27,122 @@ def test_list_submissions(db: Session, client: TestClient, logged_in_user):
     assert response.json()["results"][0]["id"] == str(submission.id)
 
 
+def test_obtain_submission_lock(db: Session, client: TestClient, logged_in_user):
+    submission = fakes.MetadataSubmissionFactory(
+        author=logged_in_user, author_orcid=logged_in_user.orcid
+    )
+    fakes.SubmissionRoleFactory(
+        submission=submission,
+        submission_id=submission.id,
+        user_orcid=logged_in_user.orcid,
+        role=SubmissionEditorRole.owner,
+    )
+    db.commit()
+
+    # Should be able to successfully GET this submission and the lock should not be set
+    response = client.request(method="get", url=f"/api/metadata_submission/{submission.id}")
+    assert response.status_code == 200
+    body = response.json()
+    assert body.get("locked_by") is None
+
+    # Attempt to acquire the lock
+    response = client.request(method="put", url=f"/api/metadata_submission/{submission.id}/lock")
+    assert response.status_code == 200
+    body = response.json()
+    assert body["success"] is True
+    assert body["locked_by"]["id"] == str(logged_in_user.id)
+
+    # Verify that the lock is set
+    response = client.request(method="get", url=f"/api/metadata_submission/{submission.id}")
+    assert response.status_code == 200
+    body = response.json()
+    assert body["locked_by"]["id"] == str(logged_in_user.id)
+
+
+def test_cannot_acquire_lock_on_locked_submission(db: Session, client: TestClient, logged_in_user):
+    locking_user = fakes.UserFactory()
+    submission = fakes.MetadataSubmissionFactory(
+        author=logged_in_user,
+        author_orcid=logged_in_user.orcid,
+        locked_by=locking_user,
+        lock_updated=datetime.utcnow(),
+    )
+    fakes.SubmissionRoleFactory(
+        submission=submission,
+        submission_id=submission.id,
+        user_orcid=logged_in_user.orcid,
+        role=SubmissionEditorRole.owner,
+    )
+    db.commit()
+
+    # Attempt to acquire the lock, verify that it fails and reports the current lock holder
+    response = client.request(method="put", url=f"/api/metadata_submission/{submission.id}/lock")
+    assert response.status_code == 409
+    body = response.json()
+    assert body["success"] is False
+    assert body["locked_by"]["id"] == str(locking_user.id)
+
+
+def test_release_submission_lock(db: Session, client: TestClient, logged_in_user):
+    submission = fakes.MetadataSubmissionFactory(
+        author=logged_in_user,
+        author_orcid=logged_in_user.orcid,
+        locked_by=logged_in_user,
+        lock_updated=datetime.utcnow(),
+    )
+    fakes.SubmissionRoleFactory(
+        submission=submission,
+        submission_id=submission.id,
+        user_orcid=logged_in_user.orcid,
+        role=SubmissionEditorRole.owner,
+    )
+    db.commit()
+
+    # Verify that the lock is set
+    response = client.request(method="get", url=f"/api/metadata_submission/{submission.id}")
+    assert response.status_code == 200
+    body = response.json()
+    assert body["locked_by"]["id"] == str(logged_in_user.id)
+
+    # Release the lock
+    response = client.request(method="put", url=f"/api/metadata_submission/{submission.id}/unlock")
+    assert response.status_code == 200
+    body = response.json()
+    assert body["success"] is True
+
+    # Verify that the lock is released
+    response = client.request(method="get", url=f"/api/metadata_submission/{submission.id}")
+    assert response.status_code == 200
+    body = response.json()
+    assert body["locked_by"] is None
+
+
+def test_cannot_release_other_users_submission_lock(
+    db: Session, client: TestClient, logged_in_user
+):
+    locking_user = fakes.UserFactory()
+    submission = fakes.MetadataSubmissionFactory(
+        author=logged_in_user,
+        author_orcid=logged_in_user.orcid,
+        locked_by=locking_user,
+        lock_updated=datetime.utcnow(),
+    )
+    fakes.SubmissionRoleFactory(
+        submission=submission,
+        submission_id=submission.id,
+        user_orcid=logged_in_user.orcid,
+        role=SubmissionEditorRole.owner,
+    )
+    db.commit()
+
+    # Attempt to release the lock, verify that it fails
+    response = client.request(method="put", url=f"/api/metadata_submission/{submission.id}/unlock")
+    assert response.status_code == 409
+    body = response.json()
+    assert body["success"] is False
+    assert body["locked_by"]["id"] == str(locking_user.id)
+
+
 def test_try_edit_locked_submission(db: Session, client: TestClient, logged_in_user):
     # Locked by a random user at utcnow by default
     submission = fakes.MetadataSubmissionFactory(

web/src/plugins/router.ts

@@ -54,6 +54,7 @@ const router = new VueRouter({
         {
           component: StepperView,
           path: '',
+          props: true,
           children: [
             {
               name: 'Submission root',

web/src/views/SubmissionPortal/Components/SubmissionList.vue

@@ -6,7 +6,7 @@ import { DataTableHeader } from 'vuetify';
 import { useRouter } from '@/use/useRouter';
 import usePaginatedResults from '@/use/usePaginatedResults';
 import {
-  loadRecord, generateRecord, submissionStatus,
+  generateRecord, submissionStatus,
 } from '../store';
 import * as api from '../store/api';
 import { HARMONIZER_TEMPLATES } from '../harmonizerApi';
@@ -71,7 +71,6 @@ export default defineComponent({
     }
 
     async function resume(item: api.MetadataSubmissionRecord) {
-      await loadRecord(item.id);
       router?.push({ name: 'Submission Context', params: { id: item.id } });
     }
 

web/src/views/SubmissionPortal/StepperView.vue

@@ -6,7 +6,6 @@ import OrcidId from '@/components/Presentation/OrcidId.vue';
 
 import { stateRefs } from '@/store';
 import { getSubmissionLockedBy } from './store';
-import { useRouter } from '@/use/useRouter';
 import { unlockSubmission } from './store/api';
 
 export default defineComponent({
@@ -19,9 +18,7 @@ export default defineComponent({
     },
   },
 
-  setup() {
-    const router = useRouter();
-
+  setup(props) {
     const loggedInUserHasLock = computed(() => {
       const lockedByUser = getSubmissionLockedBy();
       if (!lockedByUser) {
@@ -33,17 +30,12 @@ export default defineComponent({
       return false;
     });
 
-    const isEditingSubmission = computed(() => {
-      if (router) {
-        return !!router.currentRoute.params.id;
-      }
-      return false;
-    });
+    const isEditingSubmission = computed(() => props.id !== null);
 
     window.addEventListener('beforeunload', () => {
       if (isEditingSubmission.value) {
-        if (router) {
-          unlockSubmission(router.currentRoute.params.id);
+        if (props.id) {
+          unlockSubmission(props.id);
         }
       }
     });
@@ -70,9 +62,9 @@ export default defineComponent({
         :name="getSubmissionLockedBy().name"
         :authenticated="true"
       />
-      <a href="/submission/home">
+      <router-link :to="{ name: 'Submission Home'}">
         Return to submission list
-      </a>
+      </router-link>
     </v-alert>
   </div>
 </template>

web/src/views/SubmissionPortal/store/api.ts

@@ -54,6 +54,13 @@ interface PaginatedResponse<T> {
   results: T[];
 }
 
+interface LockOperationResult {
+  success: boolean;
+  message: string
+  locked_by?: User | null;
+  lock_updated?: string | null;
+}
+
 async function createRecord(record: MetadataSubmission) {
   const resp = await client.post<
     MetadataSubmissionRecord,
@@ -90,8 +97,13 @@ async function getRecord(id: string) {
   return resp.data;
 }
 
+async function lockSubmission(id: string) {
+  const resp = await client.put<LockOperationResult>(`metadata_submission/${id}/lock`);
+  return resp.data;
+}
+
 async function unlockSubmission(id: string) {
-  const resp = await client.put<string>(`metadata_submission/${id}/unlock`);
+  const resp = await client.put<LockOperationResult>(`metadata_submission/${id}/unlock`);
   return resp.data;
 }
 
@@ -104,5 +116,6 @@ export {
   getRecord,
   listRecords,
   updateRecord,
+  lockSubmission,
   unlockSubmission,
 };