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

Create null xform instances for DailyXFormSubmissionCounter #895

Merged
merged 5 commits into from
Sep 15, 2023

Conversation

LMNTL
Copy link
Contributor

@LMNTL LMNTL commented Sep 13, 2023

Description

DailyXFormSubmissionCounter doesn't actually have a pre_delete signal, so the xform=NULL counter isn't getting updated when xforms are deleted. This made sense when it got deleted after 31 days, but now it's our primary way of figuring out submission data on the usage page.

This PR resolves that by making the DailyXFormSubmissionCounter function more like the MonthlyXFormSubmissionCounter:

  • Adding a user field
  • Adding unique constraints between user, date, and xform (or just the first two when xform=NULL)
  • Adding a pre_delete signal to update the null counter

A separate PR for kpi will handle updating the shadow model (ReadOnlyDailyXFormSubmissionCounter).

Notes

I tried to update any place where daily counters are created to include the user. Because this includes the populate_submission_counters management command, I reordered migrations so that the schema changes happen before that command is run.

@LMNTL LMNTL added the backend label Sep 13, 2023
@LMNTL LMNTL changed the base branch from beta to release/2.023.37 September 13, 2023 21:28
@noliveleger noliveleger self-assigned this Sep 14, 2023
@jnm jnm requested review from jnm and removed request for noliveleger September 14, 2023 22:30
@jnm jnm assigned jnm and unassigned noliveleger Sep 14, 2023
@jnm jnm requested review from noliveleger and removed request for jnm September 15, 2023 03:41
@jnm jnm assigned noliveleger and unassigned jnm Sep 15, 2023


def add_user_to_daily_submission_counter(apps, schema_editor):
DailyXFormSubmissionCounter = apps.get_model("logger", "DailyXFormSubmissionCounter")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use single quotes ;-)



def delete_null_xform_daily_counters(apps, schema_editor):
DailyXFormSubmissionCounter = apps.get_model("logger", "DailyXFormSubmissionCounter")
Copy link
Contributor

Choose a reason for hiding this comment

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

' vs "

# add the user to the counter, based on the xform user
DailyXFormSubmissionCounter.objects.all().exclude(xform=None).update(
user=Subquery(
DailyXFormSubmissionCounter.objects.all().exclude(xform=None).values('xform__user')[:1]
Copy link
Contributor

@noliveleger noliveleger Sep 15, 2023

Choose a reason for hiding this comment

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

I think this subquery is wrong. It updates all the rows with the same user (actually that's the case when I test it).

Looking closer at the SQL query makes it more obvious.

UPDATE "logger_dailyxformsubmissioncounter"
   SET "user_id" = (
        SELECT U1."user_id"
          FROM "logger_dailyxformsubmissioncounter" U0
         INNER JOIN "logger_xform" U1
            ON (U0."xform_id" = U1."id")
         WHERE NOT (U0."xform_id" IS NULL)
         LIMIT 1
       )
 WHERE NOT ("logger_dailyxformsubmissioncounter"."xform_id" IS NULL)

Correct me if I'm wrong, but the subquery always returns the xform owner of the first daily counter.

I think you need to use a OuterRef over here.

DailyXFormSubmissionCounter.objects.all().exclude(xform=None).update(
    user=Subquery(
        DailyXFormSubmissionCounter.objects.filter(pk=OuterRef('pk')).values('xform__user')[:1]
    ),
)

With the OuterRef, the where conditions of the subquery seems more accurate.

UPDATE "logger_dailyxformsubmissioncounter"
   SET "user_id" = (
        SELECT U1."user_id"
          FROM "logger_dailyxformsubmissioncounter" U0
          LEFT OUTER JOIN "logger_xform" U1
            ON (U0."xform_id" = U1."id")
         WHERE U0."id" = "logger_dailyxformsubmissioncounter"."id"
         LIMIT 1
       )
 WHERE NOT ("logger_dailyxformsubmissioncounter"."xform_id" IS NULL)

@noliveleger noliveleger merged commit 9ed1768 into release/2.023.37 Sep 15, 2023
@noliveleger noliveleger deleted the daily-counter-null-xform branch September 15, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants