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

Add good document streaming endpoint #1818

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

kevincarrogan
Copy link
Contributor

@kevincarrogan kevincarrogan commented Feb 9, 2024

Aim

Add good document streaming endpoint.

LTD-4679

@kevincarrogan kevincarrogan force-pushed the LTD-4679-add-good-document-streaming-endpoint branch from 76ccb5b to 2ebd65a Compare February 9, 2024 11:58
@kevincarrogan kevincarrogan force-pushed the LTD-4679-add-good-document-streaming-endpoint branch 3 times, most recently from 7dc6ed7 to 89839a8 Compare February 9, 2024 14:40
@kevincarrogan kevincarrogan marked this pull request as ready for review February 9, 2024 14:46
Copy link
Contributor

@code-review-doctor code-review-doctor bot left a comment

Choose a reason for hiding this comment

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

Some things to consider. View full project report here.

api/core/tests/models.py Show resolved Hide resolved
@kevincarrogan kevincarrogan force-pushed the LTD-4679-add-good-document-streaming-endpoint branch from 89839a8 to d3afc51 Compare February 9, 2024 14:52
Copy link
Contributor

@code-review-doctor code-review-doctor bot left a comment

Choose a reason for hiding this comment

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

Worth considering. View full project report here.

api/core/tests/models.py Show resolved Hide resolved
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a5ce86b) 89.90% compared to head (b874f40) 89.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1818      +/-   ##
==========================================
+ Coverage   89.90%   89.92%   +0.01%     
==========================================
  Files         319      320       +1     
  Lines       17682    17711      +29     
  Branches     2249     2250       +1     
==========================================
+ Hits        15897    15926      +29     
  Misses       1285     1285              
  Partials      500      500              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +208 to +210
GoodStatus.SUBMITTED,
GoodStatus.QUERY,
GoodStatus.VERIFIED,
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see these are the "not draft" statuses, and that they test the logic above obj.good.status == GoodStatus.DRAFT, but I can't see here how a good gets draft status to begin with. I'm guessing it is because there is default=GoodStatus.DRAFT in the good model? and therefore it is not worth having test cases to test the document stream works for the draft status (as it is the default)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test that checks that the stream works is the first one, it doesn't explicitly set the status as it's already being set by the default value, as you said, so there is a test already. I can explicitly set this to make it clearer.

Copy link
Contributor

@hnryjmes hnryjmes 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, just added a question

Copy link
Contributor

@currycoder currycoder left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise looking good

from api.organisations.libraries.get_organisation import get_request_user_organisation_id


class IsDocumentInOrganisation(permissions.BasePermission):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to unit test these permissions

@kevincarrogan kevincarrogan force-pushed the LTD-4679-add-good-document-streaming-endpoint branch 2 times, most recently from d16bb86 to b874f40 Compare February 12, 2024 15:32
@kevincarrogan kevincarrogan merged commit 1d49554 into dev Feb 12, 2024
17 checks passed
@delete-merged-branch delete-merged-branch bot deleted the LTD-4679-add-good-document-streaming-endpoint branch February 12, 2024 16:41
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