Push notification messages #181

pxwxnvermx · 2023-10-25T06:55:18Z

calellowitz

Few questions. Overall code looks fine, just curious about various requirements mostly

calellowitz · 2023-11-02T01:40:42Z

commcare_connect/opportunity/tasks.py

+@celery_app.task()
+def send_notification_inactive_users(opportunity_id: int):
+    opportunity = Opportunity.objects.get(opportunity_id=opportunity_id)
+    opportunity_accesses = OpportunityAccess.objects.filter(opportunity_id=opportunity_id)


I think you can do this in one line. You can access the Opportunity object from the OpportunityAccess one with opportunity_access.opportunity and you can have it fetch both in one query with

OpportunityAccess.objects.filter(opportunity_id=opportunity_id).select_related("opportunity")

calellowitz · 2023-11-02T01:41:42Z

commcare_connect/opportunity/tasks.py

+        )
+        has_started_deliver = OpportunityClaim.objects.exists(access=access)
+        if (
+            last_user_learn_module.date <= datetime.date.today() - datetime.timedelta(days=3)


where does 3 come from?

It comes from the mobile user stories doc.

Maybe make these constants. You could also make a small helper:

date_before(last_user_learn_module.date, TWO_DAYS) # or date_before(last_user_learn_module.date, days=2)

I made a helper to check for dates. 0e83ab2

calellowitz · 2023-11-02T01:42:28Z

commcare_connect/opportunity/tasks.py

+            UserVisit.objects.filter(user=access.user, opportunity_id=opportunity_id).order_by("visit_date").last()
+        )
+        if (
+            last_user_deliver_visit.visit_date <= datetime.date.today() - datetime.timedelta(days=2)


where does 2 come from?

It comes fromthe mobile user stories doc.

calellowitz · 2023-11-02T01:44:17Z

commcare_connect/opportunity/tasks.py

+                title=gettext(f"Resume your job for {opportunity.name}"),
+                body=gettext(
+                    f"You have not completed your delivery visits for {opportunity.name}."
+                    "Please complete all the deliver visits to avail the payment."


Is there somewhere these messages are coming from. The language is little harsh

The body of the messages comes from me, that could be the reason they look a little unrefined. Any suggestions on how to improved these messages?

Maybe:

All deliver visits must be completed before payment can be released.

Not sure if deliver visits is the correct term. Also I thought there was payment along the way and not just at the end?

Option 2:

To maximise your payout complete all the required service delivery.

calellowitz · 2023-11-02T02:06:22Z

commcare_connect/opportunity/views.py

@@ -86,6 +89,14 @@ def get_form_kwargs(self):
    def form_valid(self, form: OpportunityCreationForm) -> HttpResponse:
        response = super().form_valid(form)
        create_learn_modules_and_deliver_units.delay(self.object.id)
+        interval = IntervalSchedule.objects.get_or_create(every=1, period=IntervalSchedule.DAYS)


Is this the recommended way to do periodic tasks? I would have imagined it was simpler to make one task and have it iterate over all opportunities, but I am not sure how to schedule that. In HQ we would use the @periodic_task decorator and give it a schedule. The other advantage of one task rather than one per domain is that it could quickly end up a very large number of tasks and clog celery.

+1 for the one task approach. You can create the schedule with a Django migration or using the post_migrate signal.

Added a migration to create the periodic task. b79b62c

snopoke · 2023-11-02T06:45:25Z

commcare_connect/opportunity/views.py

@@ -86,6 +89,14 @@ def get_form_kwargs(self):
    def form_valid(self, form: OpportunityCreationForm) -> HttpResponse:
        response = super().form_valid(form)
        create_learn_modules_and_deliver_units.delay(self.object.id)
+        interval = IntervalSchedule.objects.get_or_create(every=1, period=IntervalSchedule.DAYS)


+1 for the one task approach. You can create the schedule with a Django migration or using the post_migrate signal.

commcare_connect/opportunity/tasks.py

snopoke · 2023-11-02T06:51:20Z

commcare_connect/opportunity/tasks.py

+                title=gettext(f"Resume your job for {opportunity.name}"),
+                body=gettext(
+                    f"You have not completed your delivery visits for {opportunity.name}."
+                    "Please complete all the deliver visits to avail the payment."


Maybe:

All deliver visits must be completed before payment can be released.

Not sure if deliver visits is the correct term. Also I thought there was payment along the way and not just at the end?

Option 2:

To maximise your payout complete all the required service delivery.

snopoke · 2023-11-02T06:52:59Z

commcare_connect/opportunity/tasks.py

+        )
+        has_started_deliver = OpportunityClaim.objects.exists(access=access)
+        if (
+            last_user_learn_module.date <= datetime.date.today() - datetime.timedelta(days=3)


Maybe make these constants. You could also make a small helper:

date_before(last_user_learn_module.date, TWO_DAYS) # or date_before(last_user_learn_module.date, days=2)

snopoke · 2023-11-02T06:55:25Z

commcare_connect/opportunity/tasks.py

+                usernames=[access.user.username],
+                title=gettext(f"Resume your learning journey for {opportunity.name}"),
+                body=gettext(
+                    f"You have not completed your learning for {opportunity.name}."


What if they have completed all the modules but haven't yet claimed the opportunity? I think we're only checking when the last module was done and not whether they have done them all.

has_started_deliver indicates that if the Opportunity Claim exists or not. I have also added this check in both of the if blocks. If user has a opportunity claim then we dont need to check the learn modules.

True, but it could also be the case that the user has completed the learning modules but has not yet claimed the opportunity right?

I added this case, and refactored the code to make testing easier. f629e9e

snopoke

Few small comments but otherwise looking good.

snopoke · 2023-11-02T09:49:32Z

commcare_connect/opportunity/migrations/0025_create_send_inactive_notification_periodic_task.py

+
+
+def create_send_inactive_notification_periodic_task(apps, schema_editor):
+    interval, _ = IntervalSchedule.objects.get_or_create(every=1, period=IntervalSchedule.DAYS)


Just thought of this now. I think it would be better to use a cron schedule so that it fires at a consistent time each day, probably around 7am UTC I think.

Done. 471037d

snopoke · 2023-11-02T09:51:20Z

commcare_connect/opportunity/tasks.py

+            message = _get_deliver_message(access)
+        else:
+            message = _get_learn_message(access)
+        if isinstance(message, Message):


The possible types for message are either Message or None so I think it would be clearer to use if messages otherwise it gives the impression that there may be other types being returned.

Fixed. f629e9e

commcare_connect/opportunity/visit_import.py

…ed but not claimed case

pxwxnvermx added 4 commits October 25, 2023 12:22

Add push notification to opportunity invite

069560a

Add push notification to payment sent

895bfb3

Fix email indent

fcc1997

Fix test

d5bea57

pxwxnvermx force-pushed the pkv/push-notification-messages branch from 21bb688 to d5bea57 Compare October 25, 2023 08:53

pxwxnvermx self-assigned this Oct 25, 2023

Add user inactive notification

c94c163

pxwxnvermx marked this pull request as ready for review November 1, 2023 13:43

Add translations and updated notification texts

c8057dc

pxwxnvermx force-pushed the pkv/push-notification-messages branch from 54d55eb to c8057dc Compare November 1, 2023 13:44

pxwxnvermx requested a review from calellowitz November 1, 2023 13:45

calellowitz reviewed Nov 2, 2023

View reviewed changes

pxwxnvermx requested a review from snopoke November 2, 2023 04:24

Add fixes for date comparision, select_related opportunity

d01f1e2

snopoke reviewed Nov 2, 2023

View reviewed changes

pxwxnvermx added 3 commits November 2, 2023 14:32

Add is_date_before util

0e83ab2

Refactor send_notification_inactive_users

7176778

Add migration to create and delete periodic task

b79b62c

snopoke reviewed Nov 2, 2023

View reviewed changes

pxwxnvermx added 5 commits November 3, 2023 17:19

Refactor send_inactive_notification logic, include user learn complet…

f629e9e

…ed but not claimed case

Add tests for send inactive notification

3e74823

Add crontab based schedule for sending inactive notifications

471037d

Merge branch 'main' into pkv/push-notification-messages

2256eee

Change migration sequence after merge

8632d38

snopoke approved these changes Nov 3, 2023

View reviewed changes

pxwxnvermx added 2 commits November 7, 2023 13:25

Add check date based on equal

8029293

Fix test

c73c489

pxwxnvermx merged commit 7a0458c into main Nov 8, 2023
2 checks passed

pxwxnvermx deleted the pkv/push-notification-messages branch November 8, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push notification messages #181

Push notification messages #181

pxwxnvermx commented Oct 25, 2023

calellowitz left a comment

calellowitz Nov 2, 2023

calellowitz Nov 2, 2023

pxwxnvermx Nov 2, 2023

snopoke Nov 2, 2023

pxwxnvermx Nov 2, 2023

calellowitz Nov 2, 2023

pxwxnvermx Nov 2, 2023

calellowitz Nov 2, 2023

pxwxnvermx Nov 2, 2023

snopoke Nov 2, 2023

calellowitz Nov 2, 2023

snopoke Nov 2, 2023

pxwxnvermx Nov 2, 2023

snopoke Nov 2, 2023

snopoke Nov 2, 2023

snopoke Nov 2, 2023

snopoke Nov 2, 2023

pxwxnvermx Nov 2, 2023

snopoke Nov 2, 2023

pxwxnvermx Nov 3, 2023

snopoke left a comment

snopoke Nov 2, 2023

pxwxnvermx Nov 3, 2023

snopoke Nov 2, 2023

pxwxnvermx Nov 3, 2023



		def create_send_inactive_notification_periodic_task(apps, schema_editor):
		interval, _ = IntervalSchedule.objects.get_or_create(every=1, period=IntervalSchedule.DAYS)

Push notification messages #181

Push notification messages #181

Conversation

pxwxnvermx commented Oct 25, 2023

calellowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snopoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment