-
Notifications
You must be signed in to change notification settings - Fork 1
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
User Invite Status #305
User Invite Status #305
Conversation
on add_connect_user and send_sms
last_visit_date_d=Max("user__uservisit__visit_date", filter=Q(user__uservisit__opportunity=opportunity)), | ||
last_visit_date_d=Max( | ||
"opportunity_access__user__uservisit__visit_date", | ||
filter=Q(opportunity_access__user__uservisit__opportunity=opportunity), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this exclude trial visits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. 5014723
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit but this otherwise looks fine
@@ -125,3 +127,18 @@ def post(self, *args, **kwargs): | |||
payment.confirmation_date = now() | |||
payment.save() | |||
return Response(status=200) | |||
|
|||
|
|||
class SMSStatusCallbackView(APIView): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i don't think this belongs in opportunity API views, since this is currently only for the api the mobile uses to update its backend. I think this could just be in the normal views folder, either in users or opportunities, although i would lean toward users because that is where the other invite related views are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. 1867c9e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry left one more comment
@@ -147,15 +153,24 @@ class Meta: | |||
empty_text = "No users invited for this opportunity." | |||
orderable = False | |||
|
|||
def render_display_name(self, record): | |||
if record.opportunity_access is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is obviously a different PR, but we may need to be careful about this when the credential workflow rolls out because this would expose the phone number of anyone with a given credential (which may not be what we want to do before they accept)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I have a query regarding this. Can we show phone numbers for users who don't have a ConnectId account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. fe9a83e
I updated the code to only show phone numbers for users who have not registered on ConnectID.
|
||
|
||
def get_annotated_opportunity_access(opportunity: Opportunity): | ||
learn_modules_count = opportunity.learn_app.learn_modules.count() | ||
access_objects = ( | ||
OpportunityAccess.objects.filter(opportunity=opportunity) | ||
.select_related("user", "opportunityclaim") | ||
UserInvite.objects.filter(opportunity=opportunity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since one to one fields should be able to traverse the foreign key in either direction, it would be nice to leave this as based on the opportunity access, which would also make the diff much smaller and make the PR less risky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The User Invites can exist for users who don't have a valid connect-id. Also in cases where users don't have a connect-id account, we do not create the opportunity access that would lead to no invite status showing for them in the UserStatusTable if the model that supports it is kept to be OpportunityAccess.
Ticket