Persist submitting user metadata #563
Conversation
db/migrations/versions/2024_05_01_1700-4a0c70f02342_added_submitting_account_id_submitting_.py
Outdated
Show resolved
Hide resolved
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
from
b9b0d10 to
baba43a
Compare
Unit test has been added to make sure that new field values are saved to submission model from submission json. |
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
from
97576b6 to
92c416f
Compare
| submitting_account_id = sqla.Column(sqla.String()) | ||
| submitting_user_email = sqla.Column(sqla.String()) |
There was a problem hiding this comment.
We may want to think about whether we can backfill these columns and make them non-nullable in the future. Could you make a ticket for this and put it in the refinement bucket of one of the boards please (probably submit's board)? :)
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
from
60bf6d7 to
d924fc0
Compare
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
from
6c9a4d8 to
2d4f0c7
Compare
| ) | ||
|
|
||
| # Check that submitting_account_id and submitting_user_email are saved on the Submission | ||
| sub = Submission.query.filter_by(submission_id="S-PF-R01-1").first() |
There was a problem hiding this comment.
consider changing first() to one() - it's better practice as:
- If you are only expecting one row, one() explicitly gets this, or throws an error.
- If something changes in the future and there ends up being more than one row returned by that query (due to test fixture leakage, or badly defined test setup, or some regressive code change) then first() may give arbitrary, difficult to debug error messages, whereas one() will explicitly say it only expects one result
|
|
||
| assert response.status_code == 200, f"{response.json}" | ||
|
|
||
| sub = Submission.query.filter_by(submission_id="S-PF-R01-1").first() |
There was a problem hiding this comment.
as per other comment, switch to use one()
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
2 times, most recently
from
099aac2 to
bde4b12
Compare
| assert sub.submitting_user_email == "testuser@levellingup.gov.uk" | ||
|
|
||
|
|
||
| def test_save_submission_file_name_and_user_metadata(): |
There was a problem hiding this comment.
this needs to use a test fixture that resets the changes, made within the test, otherwise there's a big risk of the data leaking into another test (probably in a separate module, or if someone extends this one). I think in this case the changes are not committed to the DB (from what I can tell by quickly reading it) so the appropriate fixture is probably seeded_test_client_rollback
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
from
9e6d9ee to
4238299
Compare
sc15zs
force-pushed
the
persist_submitting_user_metadata
branch
from
4238299 to
88f949a
Compare
Ticket:
https://dluhcdigital.atlassian.net/browse/FMD-251
Description:
Adds two new fields to Submission model in data-store.
Saves submitting user's account_id and email to these fields passed in from post-award-submit.
PR for this:
communitiesuk/funding-service-design-post-award-submit#160
Test:
Tested locally and in dev environment.