diff --git a/backend/src/models/analysis.py b/backend/src/models/analysis.py index 3994338f..26092271 100644 --- a/backend/src/models/analysis.py +++ b/backend/src/models/analysis.py @@ -106,3 +106,29 @@ def units_to_annotate(self): }) return units + + def find_discussion_post(self, discussion_post_id): + """ + Finds a specific discussion post in an analysis by the discussion post id otherwise returns none + if no discussion with that post id in the analysis. + """ + for discussion in self.discussions: + if discussion['post_id'] == discussion_post_id: + return discussion + + return None + + def find_authored_discussion_post(self, discussion_post_id, client_id): + """ + Finds a discussion post from a user that authored the post in an analysis otherwise returns none if the post + was found, but the user did not author the post + """ + discussion_post = self.find_discussion_post(discussion_post_id) + + if discussion_post is None: + raise ValueError(f"Post '{discussion_post_id}' does not exist.") + + if discussion_post['author_id'] == client_id: + return discussion_post + + return None diff --git a/backend/src/repository/analysis_collection.py b/backend/src/repository/analysis_collection.py index 483e9f53..9a2c53d0 100644 --- a/backend/src/repository/analysis_collection.py +++ b/backend/src/repository/analysis_collection.py @@ -497,3 +497,28 @@ def add_discussion_post(self, analysis_name: str, discussion_post: object): updated_document.pop("_id", None) return updated_document['discussions'] + + def updated_discussion_post(self, discussion_post_id: str, discussion_content: str, analysis_name: str): + """ Edits a discussion post from an analysis to update the discussion post's content """ + + updated_document = self.collection.find_one_and_update({"name": analysis_name}, { + "$set": {"discussions.$[item].content": discussion_content} + }, + array_filters=[{"item.post_id": discussion_post_id}], + return_document=ReturnDocument.AFTER) + + updated_document.pop("_id", None) + + return updated_document['discussions'] + + def delete_discussion_post(self, discussion_post_id: str, analysis_name: str): + """ Removes a discussion post from an analysis """ + + updated_document = self.collection.find_one_and_update({"name": analysis_name}, { + "$pull": {"discussions": {"post_id": discussion_post_id}} + }, + return_document=ReturnDocument.AFTER) + + updated_document.pop("_id", None) + + return updated_document['discussions'] diff --git a/backend/src/routers/analysis_discussion_router.py b/backend/src/routers/analysis_discussion_router.py index c125477f..ec0cda66 100644 --- a/backend/src/routers/analysis_discussion_router.py +++ b/backend/src/routers/analysis_discussion_router.py @@ -6,10 +6,11 @@ import logging from uuid import uuid4 -from fastapi import (APIRouter, Depends, Form, Security) +from fastapi import (APIRouter, Depends, Form, Security, HTTPException, status) from ..dependencies import database from ..models.user import VerifyUser +from ..models.analysis import Analysis from ..security.security import get_current_user logger = logging.getLogger(__name__) @@ -17,32 +18,25 @@ router = APIRouter(tags=["analysis"], dependencies=[Depends(database)]) -def mock_discussion_fixture(): - """Mock dicussion fixture to fake the discussions being returned""" - return [{ - "post_id": "90jkflsd8d-6298-4afb-add5-6ef710eb5e98", "author_id": "3bghhsmnyqi6uxovazy07ryn9q1tqbnt", - "author_fullname": "Hello Person", "publish_timestamp": "2023-10-09T21:13:22.687000", - "content": "Nulla diam mi, semper vitae.", "attachments": [], "thread": [] - }, { - "post_id": "a677fdsfsacf8-4ff9-a406-b113a7952f7e", "author_id": "kw0g790fdx715xsr1ead2jk0pqubtlyz", - "author_fullname": "Developer Person", "publish_timestamp": "2023-10-10T21:13:22.687000", - "content": "Morbi laoreet justo.", "attachments": [], "thread": [] - }, { - "post_id": "e602ffdsfa-fdsfdsa-9f42-862c826255ef", "author_id": "exqkhvidr7uh2ndslsdymbzfbmqjlunk", - "author_fullname": "Variant Review Report Preparer Person", "publish_timestamp": "2023-10-13T21:13:22.687000", - "content": "Mauris pretium sem at nunc sollicitudin also.", "attachments": [], "thread": [] - }] - - @router.get("/{analysis_name}/discussions") -def get_analysis_discussions(analysis_name: str): - """ Returns a list of genomic units for a given analysis """ +def get_analysis_discussions(analysis_name: str, repositories=Depends(database)): + """ Returns a list of discussion posts for a given analysis """ logger.info("Retrieving the analysis '%s' discussions ", analysis_name) - return mock_discussion_fixture() + + found_analysis = repositories['analysis'].find_by_name(analysis_name) + + if not found_analysis: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail=f"Analysis '{analysis_name}' does not exist.'" + ) + + analysis = Analysis(**found_analysis) + + return analysis.discussions @router.post("/{analysis_name}/discussions") -def add_analysis_discussions( +def add_analysis_discussion( analysis_name: str, discussion_content: str = Form(...), repositories=Depends(database), @@ -52,6 +46,14 @@ def add_analysis_discussions( logger.info("Adding the analysis '%s' from user '%s'", analysis_name, client_id) logger.info("The message: %s", discussion_content) + found_analysis = repositories['analysis'].find_by_name(analysis_name) + + if not found_analysis: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail=f"Analysis '{analysis_name}' does not exist.'" + ) + + analysis = Analysis(**found_analysis) current_user = repositories["user"].find_by_client_id(client_id) new_discussion_post = { @@ -64,4 +66,74 @@ def add_analysis_discussions( "thread": [], } - return repositories['analysis'].add_discussion_post(analysis_name, new_discussion_post) + return repositories['analysis'].add_discussion_post(analysis.name, new_discussion_post) + + +@router.put("/{analysis_name}/discussions/{discussion_post_id}") +def update_analysis_discussion_post( + analysis_name: str, + discussion_post_id: str, + discussion_content: str = Form(...), + repositories=Depends(database), + client_id: VerifyUser = Security(get_current_user) +): + """ Updates a discussion post's content in an analysis by the discussion post id """ + logger.info( + "Editing post '%s' by user '%s' from the analysis '%s' with new content: '%s'", discussion_post_id, client_id, + analysis_name, discussion_content + ) + + found_analysis = repositories['analysis'].find_by_name(analysis_name) + + if not found_analysis: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Analysis '{analysis_name}' does not exist. Unable to update discussion post.'" + ) + + analysis = Analysis(**found_analysis) + + try: + valid_post = analysis.find_authored_discussion_post(discussion_post_id, client_id) + except ValueError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) from e + + if not valid_post: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, detail="User cannot update post they did not author." + ) + + return repositories['analysis'].updated_discussion_post(valid_post['post_id'], discussion_content, analysis.name) + + +@router.delete("/{analysis_name}/discussions/{discussion_post_id}") +def delete_analysis_discussion( + analysis_name: str, + discussion_post_id: str, + repositories=Depends(database), + client_id: VerifyUser = Security(get_current_user) +): + """ Deletes a discussion post in an analysis by the discussion post id """ + logger.info("Deleting post %s by user '%s' from the analysis '%s'", discussion_post_id, client_id, analysis_name) + + found_analysis = repositories['analysis'].find_by_name(analysis_name) + + if not found_analysis: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Analysis '{analysis_name}' does not exist. Unable to delete discussion post.'" + ) + + analysis = Analysis(**found_analysis) + + try: + valid_post = analysis.find_authored_discussion_post(discussion_post_id, client_id) + except ValueError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) from e + + if not valid_post: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, detail="User cannot delete post they did not author." + ) + + return repositories['analysis'].delete_discussion_post(valid_post['post_id'], analysis.name) diff --git a/backend/tests/integration/conftest.py b/backend/tests/integration/conftest.py index 832277d1..456560a3 100644 --- a/backend/tests/integration/conftest.py +++ b/backend/tests/integration/conftest.py @@ -74,7 +74,7 @@ def mock_current_user(): @pytest.fixture(name="mock_user") def test_auth_user(): """A mocked user that can be used to generate an OAuth2 access token""" - return {"sub": "johndoe", "scopes": ["read", "write"]} + return {"sub": "johndoe-client-id", "scopes": ["read", "write"]} @pytest.fixture(name="mock_access_token") diff --git a/backend/tests/integration/test_analysis_routers.py b/backend/tests/integration/test_analysis_routers.py index 38618b2b..87c72028 100644 --- a/backend/tests/integration/test_analysis_routers.py +++ b/backend/tests/integration/test_analysis_routers.py @@ -80,7 +80,7 @@ def test_import_analysis_with_phenotips_json( assert response.status_code == 200 response_data = json.loads(response.text) assert response_data['latest_status'] == "Preparation" - assert response_data['timeline'][0]['username'] == 'johndoe' + assert response_data['timeline'][0]['username'] == 'johndoe-client-id' def test_update_analysis_section(client, mock_access_token, mock_repositories, update_analysis_section_response_json): @@ -380,6 +380,7 @@ def valid_query_side_effect(*args, **kwargs): # pylint: disable=unused-argument return analysis mock_repositories["user"].collection.find_one.return_value = {"full_name": new_post_user} + mock_repositories['analysis'].collection.find_one.return_value = cpam0002_analysis_json mock_repositories["analysis"].collection.find_one_and_update.side_effect = valid_query_side_effect response = client.post( @@ -398,6 +399,141 @@ def valid_query_side_effect(*args, **kwargs): # pylint: disable=unused-argument assert actual_most_recent_post['content'] == new_post_content +def test_update_discussion_post_in_analysis(client, mock_access_token, mock_repositories, cpam0002_analysis_json): + """ Tests successfully updating an existing post in the discussions with the user being the author """ + cpam_analysis = "CPAM0002" + discussion_post_id = "fake-post-id" + discussion_content = "I am an integration test post. Look at me!" + + # Inject a new discussion post by John Doe + def valid_query_side_effect_one(*args, **kwargs): # pylint: disable=unused-argument + analysis = cpam0002_analysis_json + + new_discussion_post = { + "post_id": "fake-post-id", "author_id": "johndoe-client-id", "author_fullname": 'johndoe', + "content": "Hello, I am a discussion post." + } + + analysis['discussions'].append(new_discussion_post) + analysis['_id'] = 'fake-mongo-object-id' + return analysis + + def valid_query_side_effect_two(*args, **kwargs): # pylint: disable=unused-argument + find, query = args # pylint: disable=unused-variable + query_filter = kwargs + + analysis = cpam0002_analysis_json + fake_post_content = query['$set']['discussions.$[item].content'] + fake_post_id = query_filter['array_filters'][0]['item.post_id'] + + for d in analysis['discussions']: + if d['post_id'] == fake_post_id: + d['content'] = fake_post_content + + analysis['_id'] = 'fake-mongo-object-id' + + return analysis + + mock_repositories['analysis'].collection.find_one.side_effect = valid_query_side_effect_one + mock_repositories["analysis"].collection.find_one_and_update.side_effect = valid_query_side_effect_two + + response = client.put( + "/analysis/" + cpam_analysis + "/discussions/" + discussion_post_id, + headers={"Authorization": "Bearer " + mock_access_token}, + data={"discussion_content": discussion_content} + ) + + actual_post = None + + for d in response.json(): + if d['post_id'] == discussion_post_id: + actual_post = d + + assert len(response.json()) == 4 + assert actual_post['content'] == discussion_content + + +def test_update_post_in_analysis_author_mismatch(client, mock_access_token, mock_repositories, cpam0002_analysis_json): + """ Tests updating a post that the author did not post and results in an unauthorized failure """ + cpam_analysis = "CPAM0002" + discussion_post_id = "9027ec8d-6298-4afb-add5-6ef710eb5e98" + discussion_content = "I am an integration test post. Look at me!" + + mock_repositories['analysis'].collection.find_one.return_value = cpam0002_analysis_json + + response = client.put( + "/analysis/" + cpam_analysis + "/discussions/" + discussion_post_id, + headers={"Authorization": "Bearer " + mock_access_token}, + data={"discussion_content": discussion_content} + ) + + expected_failure_detail = {'detail': 'User cannot update post they did not author.'} + + assert response.status_code == 401 + assert response.json() == expected_failure_detail + + +def test_delete_discussion_post_in_analysis(client, mock_access_token, mock_repositories, cpam0002_analysis_json): + """ Tests successfully deleting an existing post in the discussions with the user being the author """ + cpam_analysis = "CPAM0002" + discussion_post_id = "fake-post-id" + + # Inject a new discussion post by John Doe + def valid_query_side_effect_one(*args, **kwargs): # pylint: disable=unused-argument + analysis = cpam0002_analysis_json + + new_discussion_post = { + "post_id": "fake-post-id", + "author_id": "johndoe-client-id", + "author_fullname": 'johndoe', + } + + analysis['discussions'].append(new_discussion_post) + analysis['_id'] = 'fake-mongo-object-id' + return analysis + + def valid_query_side_effect_two(*args, **kwargs): # pylint: disable=unused-argument + find, query = args # pylint: disable=unused-variable + + analysis = cpam0002_analysis_json + fake_post_id = query['$pull']['discussions']['post_id'] + + analysis['discussions'] = [x for x in analysis['discussions'] if fake_post_id not in x['post_id']] + analysis['_id'] = 'fake-mongo-object-id' + + return analysis + + mock_repositories['analysis'].collection.find_one.side_effect = valid_query_side_effect_one + mock_repositories["analysis"].collection.find_one_and_update.side_effect = valid_query_side_effect_two + + response = client.delete( + "/analysis/" + cpam_analysis + "/discussions/" + discussion_post_id, + headers={"Authorization": "Bearer " + mock_access_token} + ) + + assert len(response.json()) == 3 + + +def test_handle_delete_post_not_existing_in_analysis( + client, mock_access_token, mock_repositories, cpam0002_analysis_json +): + """ Tests failure of deleting a discussion post but does not exist in the analysis """ + cpam_analysis = "CPAM0002" + discussion_post_id = "fake-post-id" + + mock_repositories['analysis'].collection.find_one.return_value = cpam0002_analysis_json + + response = client.delete( + "/analysis/" + cpam_analysis + "/discussions/" + discussion_post_id, + headers={"Authorization": "Bearer " + mock_access_token} + ) + + expected_failure_detail = {'detail': f"Post '{discussion_post_id}' does not exist."} + + assert response.status_code == 404 + assert response.json() == expected_failure_detail + + @pytest.fixture(name="analysis_updates_json") def fixture_analysis_updates_json(): """The JSON that is being sent from a client to the endpoint with updates in it""" diff --git a/backend/tests/unit/models/test_analysis.py b/backend/tests/unit/models/test_analysis.py index acd166b4..e7874fad 100644 --- a/backend/tests/unit/models/test_analysis.py +++ b/backend/tests/unit/models/test_analysis.py @@ -33,6 +33,54 @@ def test_get_transcripts_in_units_to_annotate(units_to_annotate): assert "NM_001017980.3" in transcript_names +def test_find_dicussion_post(cpam0002_analysis): + """ Finds a discussion post matching the post_id """ + found_post = cpam0002_analysis.find_discussion_post("9027ec8d-6298-4afb-add5-6ef710eb5e98") + + assert found_post['author_id'] == '3bghhsmnyqi6uxovazy07ryn9q1tqbnt' + assert found_post['author_fullname'] == 'Developer Person' + + +def test_find_dicussion_post_not_found(cpam0002_analysis): + """ Finds a discussion post matching the post_id """ + found_post = cpam0002_analysis.find_discussion_post("fake-post-id-failure") + + assert found_post is None + + +def test_find_authored_discussion_post(cpam0002_analysis): + """ Tests that a discussion post is returned matching a post id and client id """ + discussion_post_id = "e6023fa7-b598-416a-9f42-862c826255ef" + client_id = 'exqkhvidr7uh2ndslsdymbzfbmqjlunk' + + found_post = cpam0002_analysis.find_authored_discussion_post(discussion_post_id, client_id) + + assert found_post['author_id'] == client_id + assert found_post['author_fullname'] == 'Variant Review Report Preparer Person' + + +def test_find_authored_discussion_post_failure_missing_post(cpam0002_analysis): + """ Tests that a ValueError is thrown if no discussion post is found matching the post_id """ + discussion_post_id = "fake-post-id-failure" + client_id = 'exqkhvidr7uh2ndslsdymbzfbmqjlunk' + + try: + cpam0002_analysis.find_authored_discussion_post(discussion_post_id, client_id) + except ValueError as error: + assert isinstance(error, ValueError) + assert str(error) == f"Post '{discussion_post_id}' does not exist." + + +def test_find_authored_discussion_post_author_mismatch(cpam0002_analysis): + """ Test that no post is returned if a post is found, but the author's client id does not match """ + discussion_post_id = "e6023fa7-b598-416a-9f42-862c826255ef" + client_id = 'fake-client-id-failure' + + found_post = cpam0002_analysis.find_authored_discussion_post(discussion_post_id, client_id) + + assert found_post is None + + @pytest.fixture(name="units_to_annotate") def fixture_units_to_annotate(cpam0002_analysis): """Fixture for the units to annotate for the CPAM0002 Analysis""" diff --git a/backend/tests/unit/repository/test_analysis_collection.py b/backend/tests/unit/repository/test_analysis_collection.py index 2af327da..465a3c44 100644 --- a/backend/tests/unit/repository/test_analysis_collection.py +++ b/backend/tests/unit/repository/test_analysis_collection.py @@ -407,6 +407,40 @@ def valid_query_side_effect(*args, **kwargs): # pylint: disable=unused-argument assert actual_most_recent_post == new_post +def test_update_discussion_post_in_analysis(analysis_collection): + """ Tests updating the content of a user's post from an analysis by the post id""" + analysis_collection.collection.find_one_and_update.return_value = {"discussions": []} + + discussion_post_id = "9027ec8d-6298-4afb-add5-6ef710eb5e98" + discussion_content = "This is new content." + analysis_name = "CPAM0002" + + expected_find = {"name": analysis_name} + expected_update = {"$set": {"discussions.$[item].content": discussion_content}} + expected_filter = [{"item.post_id": discussion_post_id}] + + analysis_collection.updated_discussion_post(discussion_post_id, discussion_content, analysis_name) + analysis_collection.collection.find_one_and_update.assert_called_once_with( + expected_find, expected_update, array_filters=expected_filter, return_document=True + ) + + +def test_delete_discussion_post_in_analysis(analysis_collection): + """ Tests deleting a user's discussion post from an analysis by the post id """ + analysis_collection.collection.find_one_and_update.return_value = {"discussions": []} + + discussion_post_id = "9027ec8d-6298-4afb-add5-6ef710eb5e98" + analysis_name = "CPAM0002" + + expected_find = {"name": analysis_name} + expected_update = {"$pull": {"discussions": {"post_id": discussion_post_id}}} + + analysis_collection.delete_discussion_post(discussion_post_id, analysis_name) + analysis_collection.collection.find_one_and_update.assert_called_with( + expected_find, expected_update, return_document=True + ) + + @pytest.fixture(name="analysis_with_no_p_dot") def fixture_analysis_with_no_p_dot(): """Returns an analysis with no p. in the genomic unit"""