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

142 all filings endpoint #341

Merged
merged 11 commits into from
Oct 11, 2024
Merged

142 all filings endpoint #341

merged 11 commits into from
Oct 11, 2024

Conversation

guffee23
Copy link
Contributor

Closes #142

Copy link

github-actions bot commented Jul 26, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/sbl_filing_api/entities/repos
  submission_repo.py
  src/sbl_filing_api/routers
  filing.py
Project Total  

This report was generated by python-coverage-comment-action

@@ -58,6 +60,13 @@ async def get_filing(request: Request, response: Response, lei: str, period_code
response.status_code = status.HTTP_204_NO_CONTENT


@router.get("/filings/{period_code}", response_model=List[FilingDTO])
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 the endpoint of /periods/{period_code}/filings might be better, since we already have a /periods prefix, and the rest of the endpoints are /institutions prefix

@@ -63,6 +63,14 @@ async def get_filing(session: AsyncSession, lei: str, filing_period: str) -> Fil
return result[0] if result else None


async def get_filings(session: AsyncSession, leis: list[str], filing_period: str) -> list[FilingDAO]:
stmt = select(FilingDAO).where(FilingDAO.lei.in_(leis)).filter_by(filing_period=filing_period)
Copy link
Collaborator

@lchen-2101 lchen-2101 Jul 26, 2024

Choose a reason for hiding this comment

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

this works, but we can also just do filter with multiple criteria

select(FilingDAO).filter(FilingDAO.lei.in_(leis), FilingDAO.filing_period == filing_period)

possible new version of query_helper to help, although I don't think a new query_helper really saves too many key strokes

Copy link
Contributor

Choose a reason for hiding this comment

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

query_helper already takes a set of kwargs using filter_by which doesn’t require filter chaining, you can pass any number of filter arguments to it

Copy link
Collaborator

Choose a reason for hiding this comment

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

if I remember correctly, filter_by has a key = value limitation; in this scenario a where in clause is needed.

@guffee23 guffee23 marked this pull request as ready for review October 10, 2024 16:31
Copy link
Collaborator

@lchen-2101 lchen-2101 left a comment

Choose a reason for hiding this comment

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

LGTM

@lchen-2101 lchen-2101 merged commit 06bb249 into main Oct 11, 2024
5 checks passed
@lchen-2101 lchen-2101 deleted the 142_all_filings_endpoint branch October 11, 2024 14:28
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.

[post-mvp] Add endpoint to return all filings for a given user for a given filing period
3 participants