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 endpoint to stream document on organisation #1809

Merged

Conversation

kevincarrogan
Copy link
Contributor

@kevincarrogan kevincarrogan commented Feb 6, 2024

Aim

Add endpoint to stream document on organisation

LTD-4672

@kevincarrogan kevincarrogan force-pushed the LTD-4672-organisation-document-streaming-endpoint branch from 5f0bb3c to ddb0799 Compare February 6, 2024 14:52
@@ -1078,6 +1079,23 @@ def add_users(self, count=3):
out.append(user)
return out

def create_default_bucket(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

probably silly question but this is to create our bucket locally MIMO type for testing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It creates a bucket that is for the tests.

If you look at the test case that I've added it uses the mock_aws which is using an in-memory mock of S3 so this isn't testing any external buckets.



class DocumentOnOrganisationStreamView(RetrieveAPIView):
authentication_classes = (SharedAuthentication,)
Copy link
Contributor

Choose a reason for hiding this comment

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

as a precaution when user is an exporter is it worth checking they're part of the organisation.
I don't think we doing these kind of checked but wondering if it's worth introducing these checks

Copy link
Contributor

Choose a reason for hiding this comment

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

oh we do have a OrganisationAuthentication
to do just that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. OrganisationAuthentication isn't the fix for that.

@kevincarrogan kevincarrogan force-pushed the LTD-4672-organisation-document-streaming-endpoint branch from ddb0799 to 9920abd Compare February 6, 2024 16:07
@kevincarrogan kevincarrogan force-pushed the LTD-4672-organisation-document-streaming-endpoint branch from 9920abd to af00f54 Compare February 6, 2024 16:16
Comment on lines 30 to 33
def retrieve(self, request, pk, document_on_application_pk):
instance = get_object_or_404(self.get_queryset(), pk=document_on_application_pk)
instance = self.get_object()
serializer = self.serializer_class(instance)
return JsonResponse(serializer.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

with the instance = self.get_object() change this is pretty much the same as method in base class RetrieveModelMixin, so is this required?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh we need JsonResponse which is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can actually remove def retrieve I did that very thing but then thought it might confuse people as to why all the others have been overridden and this one wasn't. I just imagined a PR review saying "why did you remove this method".

@kevincarrogan kevincarrogan force-pushed the LTD-4672-organisation-document-streaming-endpoint branch from b8cac6f to aa7dfcb Compare February 6, 2024 18:28
Copy link
Contributor

@depsiatwal depsiatwal left a comment

Choose a reason for hiding this comment

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

lgtm

@kevincarrogan kevincarrogan force-pushed the LTD-4672-organisation-document-streaming-endpoint branch from a338054 to 9caf6ac Compare February 6, 2024 19:05
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0ce06e9) 60.54% compared to head (9caf6ac) 90.15%.

Additional details and impacted files
@@             Coverage Diff             @@
##              dev    #1809       +/-   ##
===========================================
+ Coverage   60.54%   90.15%   +29.60%     
===========================================
  Files         317      320        +3     
  Lines       17694    17764       +70     
  Branches     2260     2263        +3     
===========================================
+ Hits        10713    16015     +5302     
+ Misses       6579     1248     -5331     
- Partials      402      501       +99     

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

@kevincarrogan kevincarrogan merged commit 7063a06 into dev Feb 7, 2024
17 checks passed
@delete-merged-branch delete-merged-branch bot deleted the LTD-4672-organisation-document-streaming-endpoint branch February 7, 2024 12:39
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.

None yet

3 participants