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

include submission time slot in opportunity API #413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sravfeyn
Copy link
Member

@sravfeyn sravfeyn commented Oct 9, 2024

No description provided.

@calellowitz
Copy link
Collaborator

This PR needs a description

Copy link
Collaborator

@calellowitz calellowitz left a comment

Choose a reason for hiding this comment

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

Few questions, beyond the request for a description

@@ -178,6 +185,18 @@ def get_catchment_areas(self, obj):
catchments = CatchmentArea.objects.filter(opportunity_access=opp_access)
return CatchmentAreaSerializer(catchments, many=True).data

@lru_cache
def _get_flags(self, obj):
return OpportunityVerificationFlags.objects.filter(opportunity=obj).first()
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this use .get instead or even obj.opportunityverificationflags? It's a OneToOne model, we can guarantee there is only one so filter and first seem unnecessary. Given that we can use normal accessor syntax, this whole cached get may be unnecessary, since it will be loaded the first time its accessed and already saved on the object the later times (rather than saved in redis)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I totally overlooked it. I have updated the code and also addressed the rest of the comments too.

@@ -105,6 +108,8 @@ class OpportunitySerializer(serializers.ModelSerializer):
payment_units = serializers.SerializerMethodField()
is_user_suspended = serializers.SerializerMethodField()
catchment_areas = serializers.SerializerMethodField()
start_time_threshold = serializers.SerializerMethodField()
Copy link
Collaborator

Choose a reason for hiding this comment

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

As an extension of my comment below, I am not sure this needs a method rather than just the source argument. Alternatively, I think this might be nicer in a new verification_flags object, rather than at the top level, since we are likely to add the others and it will help with organization.

@@ -133,6 +138,8 @@ class Meta:
"payment_units",
"is_user_suspended",
"catchment_areas",
"start_time_threshold",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason to change the names here? It obviously doesn't really matter but it does add a tiny bit of inconsistency between what the mobile and web team will call these which can be annoying.

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.

2 participants