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

update submission route #189

Merged
merged 24 commits into from
Aug 22, 2024
Merged

update submission route #189

merged 24 commits into from
Aug 22, 2024

Conversation

BrandonSharratt
Copy link
Collaborator

*Issue:*bcgov/entity#22381

Description of changes:
Added a util function for deep_spread and created an update route for submissions (PUT /:id) Added unit tests for deep spread

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).

… and store submitted payload instead of previous data
@BrandonSharratt BrandonSharratt self-assigned this Aug 19, 2024
@BrandonSharratt
Copy link
Collaborator Author

/gcbrun

@@ -94,6 +98,8 @@ def get_entity_submission(business_identifier: str):

submission = SubmissionModel.find_by_business_identifier(business_identifier)
if submission:
if submission.submitted_payload is None or submission.submitted_payload == '':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a need to set this on GET method ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We probably don't need it long term, I just didn't want to return no information for a field that SHOULD have information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would remove it, as this is still all dev and no wrong data is in production.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I would also prefer these 2 GET endpoints not set the submitted_payload

btr-api/src/btr_api/services/submission.py Show resolved Hide resolved
@@ -75,12 +76,45 @@ def create_submission(submission_dict: dict, submitter_id: int) -> SubmissionMod

"""
submission = SubmissionModel.find_by_business_identifier(submission_dict['businessIdentifier'])
if not submission:
submission = SubmissionModel()
submission.business_identifier = submission_dict['businessIdentifier']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line got accidentally merged incorrectly, we need to set identifier for submission too after creating new model.

Copy link
Collaborator

@kialj876 kialj876 left a comment

Choose a reason for hiding this comment

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

Hey Brandon, nice work. I would like a unit test like I mentioned below and also a postman test for the new endpoint, but other than that this looks good to me 👍

@@ -13,6 +13,7 @@
from sqlalchemy import text
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test in here similar to 'test_post_plots' for the put? Ideally it would start with a realistic example of an SI and then the PUT would update an existing person with things like name, etc. and then also add a new person, and then it would verify the result is how we want the SI to look

Copy link
Collaborator

@hfekete hfekete left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for all the updates !

try:
user = UserModel.get_or_create_user_by_jwt(g.jwt_oidc_token_info)
account_id = request.headers.get('Account-Id', None)
submission = SubmissionModel.find_by_id(sub_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have submission ID in the UI already, or do we need ticket/update to get that information ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have it in the UI already

@BrandonSharratt BrandonSharratt merged commit 8ea8f5a into bcgov:main Aug 22, 2024
4 checks passed
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.

3 participants