Skip to content

Conversation

@PixelDust22
Copy link
Contributor

@PixelDust22 PixelDust22 commented Aug 7, 2021

Added endpoint to query multiple PromptActivity features at once.

  • Tests

@PixelDust22 PixelDust22 requested a review from scefali August 7, 2021 00:49
conditions = condition if conditions is None else (conditions | condition)
if conditions is None:
return Response({"detail": "No feature specified"}, status=400)
result = PromptsActivity.objects.filter(conditions, user=request.user).all()
Copy link
Contributor

Choose a reason for hiding this comment

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

@Neo-Zhixing Nit: I don't think you need .all() here

Copy link
Contributor

@scefali scefali left a comment

Choose a reason for hiding this comment

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

LGTM, just need tests

EDIT: added an additional comment

class PromptsActivitiesEndpoint(Endpoint):
permission_classes = (IsAuthenticated,)

def post(self, request):
Copy link
Contributor

@scefali scefali Aug 12, 2021

Choose a reason for hiding this comment

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

@Neo-Zhixing just noticed this is a post request. Would be nice if we could do this as get request. Could this endpoint take a list of features and list of fields so we could handle this in a get? I don't think we need to map feature too field (we just pass all the fields we need for all the features feature and pick the ones we need based on required_fields)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like if this is what we're going to do, we could totally reuse and modify the old end point so that it accepts multiple features.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Neo-Zhixing The problem with that approach is that we'll have two different return types. I'm ok with it but make sure you update the PromptResponse type to reflect that it could be an array in your PR.

data = None if result is None else result.data
return Response({"data": data, "features": featuredata})
else:
return Response({"features": featuredata})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to maintain backward compatibility with a bunch of tests that mocks the endpoint with {data: xxx}.
Long term we might want to update this so that we can support both promptsCheck and batchedPromptsCheck with a uniform behavior, regardless of the number of features requested.

return Response({})

return Response({"data": result.data})
features = request.GET.getlist("feature")
Copy link
Contributor

Choose a reason for hiding this comment

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

@Neo-Zhixing Nicely done using getlist here :)

if len(features) == 1:
result = result.first()
data = None if result is None else result.data
return Response({"data": data, "features": featuredata})
Copy link
Contributor

Choose a reason for hiding this comment

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

@Neo-Zhixing excellent idea also setting features even though we aren't using it, it will make future changes much easier

Copy link
Contributor

@scefali scefali left a comment

Choose a reason for hiding this comment

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

LGTM

@PixelDust22 PixelDust22 merged commit 660c923 into master Aug 14, 2021
@PixelDust22 PixelDust22 deleted the feat/prompt-activites-endpoint branch August 14, 2021 00:04
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants