-
Notifications
You must be signed in to change notification settings - Fork 1
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
96 add filing sign endpoint #130
Changes from 22 commits
54e5307
70e1b20
c1cd894
2593be9
94a60a5
ddf578a
c25a848
76f5c41
ec77581
9292660
19030ea
c905ee9
48cb342
2b13a94
27424af
6103936
1649f36
a2c2177
4abd730
942917a
f7ac3be
a7a9a08
3d5b12c
3a7b578
3573cca
52a10dc
228b847
1256ea8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
"""add signature table | ||
|
||
Revision ID: fb46d55283d6 | ||
Revises: 7a1b7eab0167 | ||
Create Date: 2024-03-13 11:41:47.815220 | ||
|
||
""" | ||
from typing import Sequence, Union | ||
|
||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
# revision identifiers, used by Alembic. | ||
revision: str = "fb46d55283d6" | ||
down_revision: Union[str, None] = "7a1b7eab0167" | ||
branch_labels: Union[str, Sequence[str], None] = None | ||
depends_on: Union[str, Sequence[str], None] = None | ||
|
||
|
||
def upgrade() -> None: | ||
op.create_table( | ||
"signature", | ||
sa.Column("id", sa.INTEGER, autoincrement=True), | ||
sa.Column("filing", sa.Integer), | ||
sa.Column("signer_id", sa.String, nullable=False), | ||
sa.Column("signer_name", sa.String), | ||
sa.Column("signed_date", sa.DateTime(), server_default=sa.func.now(), nullable=False), | ||
sa.PrimaryKeyConstraint("id", name="signature_pkey"), | ||
sa.ForeignKeyConstraint(["filing"], ["filing.id"], name="signature_filing_fkey"), | ||
) | ||
|
||
|
||
def downgrade() -> None: | ||
op.drop_table("signature") |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -59,6 +59,34 @@ async def post_filing(request: Request, lei: str, period_code: str): | |||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@router.put("/institutions/{lei}/filings/{period_code}/sign", response_model=FilingDTO) | ||||||||||||||||||||||||||||||||||||||||||
@requires("authenticated") | ||||||||||||||||||||||||||||||||||||||||||
async def sign_filing(request: Request, lei: str, period_code: str): | ||||||||||||||||||||||||||||||||||||||||||
filing = await repo.get_filing(request.state.db_session, lei, period_code) | ||||||||||||||||||||||||||||||||||||||||||
if not filing: | ||||||||||||||||||||||||||||||||||||||||||
return JSONResponse( | ||||||||||||||||||||||||||||||||||||||||||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||||||||||||||||||||||||||||||||||||||||||
content=f"There is no Filing for LEI {lei} in period {period_code}, unable to sign a non-existent Filing.", | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
latest_sub = await repo.get_latest_submission(request.state.db_session, lei, period_code) | ||||||||||||||||||||||||||||||||||||||||||
if not latest_sub or latest_sub.state != SubmissionState.SUBMISSION_ACCEPTED: | ||||||||||||||||||||||||||||||||||||||||||
return JSONResponse( | ||||||||||||||||||||||||||||||||||||||||||
status_code=status.HTTP_403_FORBIDDEN, | ||||||||||||||||||||||||||||||||||||||||||
content=f"Cannot sign filing. Filing for {lei} for period {period_code} does not have a latest submission the SUBMISSION_ACCEPTED state.", | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
if not filing.contact_info: | ||||||||||||||||||||||||||||||||||||||||||
return JSONResponse( | ||||||||||||||||||||||||||||||||||||||||||
status_code=status.HTTP_403_FORBIDDEN, | ||||||||||||||||||||||||||||||||||||||||||
content=f"Cannot sign filing. Filing for {lei} for period {period_code} does not have contact info defined.", | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+87
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to this Contact Info check, do we need something similar for institution data? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at where the filing is originally created, it looks like we're still mocking that Institution Snapshot data. sbl-filing-api/src/sbl_filing_api/entities/repos/submission_repo.py Lines 130 to 138 in a7a9a08
So, it seems like there'll always be a snapshot value set, but it's not currently valid? Also, I see we have a sbl-filing-api/src/sbl_filing_api/routers/filing.py Lines 161 to 171 in a7a9a08
...but I'm not sure where in the current flow we'd call that. Around when we set the SBL institution type? And how would we know to call it if there's newer/different institution data? Lookup the latest one the Sign and submit page, updating it if there is one? ...and do we need to solve any of this for MVP? Is it good enough to just show the latest version of institution data on sign and submit? Sorry, I know this is now off-topic for this PR. 😆 I'll create a separate issue to work through what (if anything) we want to do for MVP. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I thought about this. Right now we set the institution snapshot ID to a default of "v1". But I think we want to have that start out, like contact info, as None and when they check or update their institution data, the FE sets the snapshot ID and that's how we know they've completed that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created story #143 |
||||||||||||||||||||||||||||||||||||||||||
sig = await repo.add_signature( | ||||||||||||||||||||||||||||||||||||||||||
request.state.db_session, signer_id=request.user.id, signer_name=request.user.name, filing_id=filing.id | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
filing.confirmation_id = lei + "-" + period_code + "-" + str(latest_sub.id) + "-" + str(sig.signed_date.timestamp()) | ||||||||||||||||||||||||||||||||||||||||||
filing.signatures.append(sig) | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+93
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we know when a filing needs to be re-signed once changes have been made to an already-signed filing? We could...
Anyway, I don't think we need to solve this for this PR, but I think we will before we go live. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created #146 |
||||||||||||||||||||||||||||||||||||||||||
return await repo.upsert_filing(request.state.db_session, filing) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@router.post("/institutions/{lei}/filings/{period_code}/submissions", response_model=SubmissionDTO) | ||||||||||||||||||||||||||||||||||||||||||
@requires("authenticated") | ||||||||||||||||||||||||||||||||||||||||||
async def upload_file( | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -123,7 +151,7 @@ async def accept_submission(request: Request, id: int, lei: str, period_code: st | |||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||
return JSONResponse( | ||||||||||||||||||||||||||||||||||||||||||
status_code=status.HTTP_403_FORBIDDEN, | ||||||||||||||||||||||||||||||||||||||||||
content=f"Submission {id} for LEI {lei} in filing period {period_code} is not in an acceptable state. Submissions must be validated successfully or with only warnings to be signed", | ||||||||||||||||||||||||||||||||||||||||||
content=f"Submission {id} for LEI {lei} in filing period {period_code} is not in an acceptable state. Submissions must be validated successfully or with only warnings to be accepted.", | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
result.state = SubmissionState.SUBMISSION_ACCEPTED | ||||||||||||||||||||||||||||||||||||||||||
result.accepter = request.user.id | ||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ def auth_mock(mocker: MockerFixture) -> Mock: | |
@pytest.fixture | ||
def authed_user_mock(auth_mock: Mock) -> Mock: | ||
claims = { | ||
"name": "test", | ||
"name": "Test User", | ||
"preferred_username": "test_user", | ||
"email": "[email protected]", | ||
"institutions": ["123456ABCDEF", "654321FEDCBA"], | ||
|
@@ -72,7 +72,7 @@ def get_filing_mock(mocker: MockerFixture) -> Mock: | |
mock = mocker.patch("sbl_filing_api.entities.repos.submission_repo.get_filing") | ||
mock.return_value = FilingDAO( | ||
id=1, | ||
lei="1234567890", | ||
lei="123456ABCDEF", | ||
filing_period="2024", | ||
institution_snapshot_id="v1", | ||
contact_info=ContactInfoDAO( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import pytest | ||
|
||
from copy import deepcopy | ||
from datetime import datetime as dt | ||
|
||
from unittest.mock import ANY, Mock, AsyncMock | ||
|
||
|
@@ -18,6 +19,7 @@ | |
FilingTaskState, | ||
ContactInfoDAO, | ||
FilingDAO, | ||
SignatureDAO, | ||
) | ||
from sbl_filing_api.entities.models.dto import ContactInfoDTO | ||
|
||
|
@@ -50,14 +52,14 @@ def test_unauthed_get_filing(self, app_fixture: FastAPI, get_filing_mock: Mock): | |
|
||
def test_get_filing(self, app_fixture: FastAPI, get_filing_mock: Mock, authed_user_mock: Mock): | ||
client = TestClient(app_fixture) | ||
res = client.get("/v1/filing/institutions/1234567890/filings/2024/") | ||
get_filing_mock.assert_called_with(ANY, "1234567890", "2024") | ||
res = client.get("/v1/filing/institutions/123456ABCDEF/filings/2024/") | ||
get_filing_mock.assert_called_with(ANY, "123456ABCDEF", "2024") | ||
assert res.status_code == 200 | ||
assert res.json()["lei"] == "1234567890" | ||
assert res.json()["lei"] == "123456ABCDEF" | ||
assert res.json()["filing_period"] == "2024" | ||
|
||
get_filing_mock.return_value = None | ||
res = client.get("/v1/filing/institutions/1234567890/filings/2024/") | ||
res = client.get("/v1/filing/institutions/123456ABCDEF/filings/2024/") | ||
assert res.status_code == 204 | ||
|
||
def test_unauthed_post_filing(self, app_fixture: FastAPI): | ||
|
@@ -211,7 +213,7 @@ def test_authed_upload_file( | |
files = {"file": ("submission.csv", open(submission_csv, "rb"))} | ||
client = TestClient(app_fixture) | ||
|
||
res = client.post("/v1/filing/institutions/1234567890/filings/2024/submissions", files=files) | ||
res = client.post("/v1/filing/institutions/123456ABCDEF/filings/2024/submissions", files=files) | ||
mock_add_submission.assert_called_with(ANY, 1, "123456-7890-ABCDEF-GHIJ", "submission.csv") | ||
assert mock_update_submission.call_args.args[0].state == SubmissionState.SUBMISSION_UPLOADED | ||
assert res.status_code == 200 | ||
|
@@ -544,7 +546,7 @@ async def test_accept_submission(self, mocker: MockerFixture, app_fixture: FastA | |
assert res.status_code == 403 | ||
assert ( | ||
res.json() | ||
== "Submission 1 for LEI 1234567890 in filing period 2024 is not in an acceptable state. Submissions must be validated successfully or with only warnings to be signed" | ||
== "Submission 1 for LEI 1234567890 in filing period 2024 is not in an acceptable state. Submissions must be validated successfully or with only warnings to be accepted." | ||
) | ||
|
||
mock.return_value.state = SubmissionState.VALIDATION_SUCCESSFUL | ||
|
@@ -558,3 +560,95 @@ async def test_accept_submission(self, mocker: MockerFixture, app_fixture: FastA | |
res = client.put("/v1/filing/institutions/1234567890/filings/2024/submissions/1/accept") | ||
assert res.status_code == 422 | ||
assert res.json() == "Submission ID 1 does not exist, cannot accept a non-existing submission." | ||
|
||
async def test_good_sign_filing( | ||
self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock, get_filing_mock: Mock | ||
): | ||
mock = mocker.patch("sbl_filing_api.entities.repos.submission_repo.get_latest_submission") | ||
mock.return_value = SubmissionDAO( | ||
id=1, | ||
submitter="[email protected]", | ||
filing=1, | ||
state=SubmissionState.SUBMISSION_ACCEPTED, | ||
validation_ruleset_version="v1", | ||
submission_time=datetime.datetime.now(), | ||
filename="file1.csv", | ||
) | ||
|
||
add_sig_mock = mocker.patch("sbl_filing_api.entities.repos.submission_repo.add_signature") | ||
add_sig_mock.return_value = SignatureDAO( | ||
id=1, | ||
filing=1, | ||
signer_id="1234", | ||
signer_name="Test user", | ||
signed_date=datetime.datetime.now(), | ||
) | ||
|
||
upsert_mock = mocker.patch("sbl_filing_api.entities.repos.submission_repo.upsert_filing") | ||
updated_filing_obj = deepcopy(get_filing_mock.return_value) | ||
upsert_mock.return_value = updated_filing_obj | ||
|
||
client = TestClient(app_fixture) | ||
res = client.put("/v1/filing/institutions/123456ABCDEF/filings/2024/sign") | ||
add_sig_mock.assert_called_with(ANY, signer_id="123456-7890-ABCDEF-GHIJ", signer_name="Test User", filing_id=1) | ||
assert upsert_mock.call_args.args[1].confirmation_id.startswith("123456ABCDEF-2024-1-") | ||
assert res.status_code == 200 | ||
assert float(upsert_mock.call_args.args[1].confirmation_id.split("-")[3]) == pytest.approx( | ||
dt.now().timestamp(), abs=1.5 | ||
) | ||
|
||
async def test_errors_sign_filing( | ||
self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock, get_filing_mock: Mock | ||
): | ||
sub_mock = mocker.patch("sbl_filing_api.entities.repos.submission_repo.get_latest_submission") | ||
sub_mock.return_value = SubmissionDAO( | ||
id=1, | ||
submitter="[email protected]", | ||
filing=1, | ||
state=SubmissionState.VALIDATION_SUCCESSFUL, | ||
validation_ruleset_version="v1", | ||
submission_time=datetime.datetime.now(), | ||
filename="file1.csv", | ||
) | ||
|
||
client = TestClient(app_fixture) | ||
res = client.put("/v1/filing/institutions/123456ABCDEF/filings/2024/sign") | ||
assert res.status_code == 403 | ||
assert ( | ||
res.json() | ||
== "Cannot sign filing. Filing for 123456ABCDEF for period 2024 does not have a latest submission the SUBMISSION_ACCEPTED state." | ||
) | ||
|
||
sub_mock.return_value = None | ||
res = client.put("/v1/filing/institutions/123456ABCDEF/filings/2024/sign") | ||
assert res.status_code == 403 | ||
assert ( | ||
res.json() | ||
== "Cannot sign filing. Filing for 123456ABCDEF for period 2024 does not have a latest submission the SUBMISSION_ACCEPTED state." | ||
) | ||
|
||
sub_mock.return_value = SubmissionDAO( | ||
id=1, | ||
submitter="[email protected]", | ||
filing=1, | ||
state=SubmissionState.SUBMISSION_ACCEPTED, | ||
validation_ruleset_version="v1", | ||
submission_time=datetime.datetime.now(), | ||
filename="file1.csv", | ||
) | ||
|
||
get_filing_mock.return_value.contact_info = None | ||
res = client.put("/v1/filing/institutions/123456ABCDEF/filings/2024/sign") | ||
assert res.status_code == 403 | ||
assert ( | ||
res.json() | ||
== "Cannot sign filing. Filing for 123456ABCDEF for period 2024 does not have contact info defined." | ||
) | ||
|
||
get_filing_mock.return_value = None | ||
res = client.put("/v1/filing/institutions/123456ABCDEF/filings/2024/sign") | ||
assert res.status_code == 422 | ||
assert ( | ||
res.json() | ||
== "There is no Filing for LEI 123456ABCDEF in period 2024, unable to sign a non-existent Filing." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding email address while we're at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to add the signer email with some small rewrites to support that