Model changes for "form-submitting mobile workers" invoicing #34847

nospame · 2024-07-02T19:24:38Z

Product Description

No visible changes.

Technical Summary

SAAS-15705, SAAS-15706
Adds "Form-Submitting Mobile Worker" FeatureType choice which could be added to a software plan. Adds FormSubmittingMobileWorkerHistory to store the number of form-submitting mobile workers for a domain in a given month, which will be used for invoicing based on this feature type.

Feature Flag

Not feature flagged. This will have no visible effect until the feature is added to a software plan. This could be feature flagged for development if reviewers have any concern about accidentally adding to a software plan (unlikely but technically possible) before it is fully functional.

Safety Assurance

Safety story

Tested migrations locally in forward and reverse, and basic migration functionality is tested implicitly every time db setup runs for tests.

Automated test coverage

No automated tests added here, but some changes covered by test_ensure_plans.

QA Plan

No QA planned for this portion of feature development.

Migrations

The migrations in this code can be safely applied first independently of the code

Rollback instructions

If rollback is necessary, migrations should be rolled back individually to ensure operations complete in the correct order. By default, django will attempt to perform migration operations all together, which results in "Form-Submitting Mobile Worker" existing in the DB as an invalid Feature or FeatureRate.

manage.py migrate accounting 0096
manage.py migrate accounting 0095

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

Prevents migration failures from trying to ensure a feature_type that violates a previous max_length constraint

- Remove implication of privacy re: leading _ - Remove unused edition argument

…ore models

dannyroberts · 2024-07-09T15:40:33Z

corehq/apps/accounting/models.py

+class DomainSubmittingMobileWorkerHistory(DomainUserHistoryBase):
+    """
+    The number of form-submitting mobile workers in a domain for the month
+    preceeding the record date.


Can you be explicit in the comment about what a month means in this context? 30 days? 31 days? However many days the current month is long? The full previous calendar month (i.e. not up through to day? The current calendar month up through today?

The full previous calendar month (i.e. not up through to day?

The intent is for it to be this one, using the calculate form-submitting mobile workers task that will be run on 1st of every month. There's nothing in the model limiting it to that though, and the way the task will likely be written (mirroring other similar invoicing functions), running the task on say, August 9th would use data from dates July 9th to August 8th, inclusive.

Is it worth being that explicit in the comment, or is it reasonable just to call out the intended, practical use here?

I think it's worth being explicit in the comment

dannyroberts · 2024-07-09T15:44:29Z

corehq/apps/accounting/bootstrap/config/submitting_mobile_worker_feature_rate.py

+
+BOOTSTRAP_CONFIG = {
+    "feature_rates": {
+        FeatureType.SUBMITTING_MOBILE_WORKER: dict(monthly_limit=2000, per_excess_fee=Decimal('3.00'))


I realize this is possibly the most annoying type of feedback but—would you be open to changing the canonical name of this in both code and translated human text from "submitting mobile worker" to "form-submitting mobile worker" (form_submitting_mobile_worker, FORM_SUBMITTING_MOBILE_WORKER, "Form-Submitting Mobile Worker", etc.)? I realize it's kinda long, but I think it's worth being consistent (currently both are used in this code, and form-submitting is what we've been saying out loud to each other) and form-submitting is more explicit/harder to be confused about what it means.

I agree it's clearer, was just worried about being overly verbose. I can make that change.

I had originally suggested "Submitting Mobile Worker" as a replacement for "Active User" which—also leaning to something less verbose but still more descriptive. However, I second Danny's suggestion here to be more verbose and err on the side of extra clarity for the longer term :)

What do we think about DomainSubmittingMobileWorkerHistory? Does that become DomainFormSubmittingMobileWorkerHistory in the change as well, or can it possibly drop Domain and just become FormSubmittingMobileWorkerHistory since domain is implied by subclassing DomainUserHistoryBase?

I think it's fine to drop the Domain in the name. In favor of FormSubmittingMobileWorkerHistory

biyeun · 2024-07-09T18:43:12Z

corehq/apps/accounting/bootstrap/utils.py

-        # Don't prefix web user feature name with edition
-        if feature_type == FeatureType.WEB_USER:
+    for feature_type in feature_rates.keys():
+        if feature_type not in FeatureType.EDITIONED_FEATURES:


i know the original line was added when the WEB_USER feature was introduced, but to me it always makes more sense for readability to have a true statement for the limited set, and then else catch everything else. I would rewrite this block to instead be

if feature_type in FeatureType.EDITIONED_FEATURES: feature = Feature(name=f"{feature_type} {edition}", feature_type=feature_type) else: feature = Feature(name=feature_type, feature_type=feature_type)

I went a slightly different route after noticing we were instantiating a Feature whether or not it was necessary. Still took the idea of using an if rather than an if not. 88b54f8

biyeun

left a few comments on verbiage, but otherwise functionally looks good :)

biyeun · 2024-07-09T20:53:49Z

corehq/apps/accounting/bootstrap/utils.py

+        feature_name = feature_type
+        if feature_type in FeatureType.EDITIONED_FEATURES:
+            feature_name = f"{feature_name} {edition}"
            if edition == SoftwarePlanEdition.ENTERPRISE:
-                feature.name = f"Dimagi Only {feature.name}"
+                feature_name = f"Dimagi Only {feature_name}"
        try:
-            feature = Feature.objects.get(name=feature.name)
+            feature = Feature.objects.get(name=feature_name)
            if verbose:
                log_accounting_info(
                    f"Feature '{feature.name}' already exists. Using existing feature to add rate."
                )
        except Feature.DoesNotExist:
-            feature.save()
+            feature = Feature.objects.create(name=feature_name, feature_type=feature_type)


even better. thanks for this refactor

biyeun · 2024-07-09T20:58:55Z

corehq/apps/accounting/bootstrap/utils.py

+        feature_name = feature_type
+        if feature_type in FeatureType.EDITIONED_FEATURES:
+            feature_name = f"{feature_name} {edition}"
            if edition == SoftwarePlanEdition.ENTERPRISE:
-                feature.name = f"Dimagi Only {feature.name}"
+                feature_name = f"Dimagi Only {feature_name}"


with the nested ifs i find it easier to read the following...

if feature_type in FeatureType.EDITIONED_FEATURES: feature_name = (f"Dimagi Only {feature_name}" if edition == SoftwarePlanEdition.ENTERPRISE else f"{feature_name} {edition}")

otherwise, having the else: after the second nested if helps contain the nest...
and then assign feature_name = feature_type there

I don't personally love the long inline conditionals like that but happy to add an else if it seems clearer. 1233633

…obile workers' everywhere

biyeun

Thanks Evan!

nospame added reindex/migration Reindex or migration will be required during or before deploy product/invisible Change has no end-user visible impact labels Jul 2, 2024

nospame requested a review from jingcheng16 July 2, 2024 19:24

nospame marked this pull request as ready for review July 2, 2024 21:05

nospame requested review from dannyroberts and biyeun as code owners July 2, 2024 21:05

nospame marked this pull request as draft July 4, 2024 15:10

nospame added 5 commits July 4, 2024 10:30

Model changes to support Submitting Mobile Workers invoicing

1f763d0

Only 'ensure' features that exist in plan details

8c2f148

Prevents migration failures from trying to ensure a feature_type that violates a previous max_length constraint

Add feature and feature rate for Submitting Mobile Worker

652faed

Minor refactor ensure_feature_rates

355468d

- Remove implication of privacy re: leading _ - Remove unused edition argument

Fix test failure by adding DomainSubmittingMobileWorkerHistory to ign…

6e0643f

…ore models

nospame force-pushed the em/active-users-invoicing-model-changes branch from 885367e to 6e0643f Compare July 4, 2024 17:39

nospame changed the title ~~Model changes for active mobile users invoicing~~ Model changes for "submitting mobile workers" invoicing Jul 4, 2024

nospame marked this pull request as ready for review July 4, 2024 18:13

dannyroberts reviewed Jul 9, 2024

View reviewed changes

biyeun reviewed Jul 9, 2024

View reviewed changes

Refactor feature_name construction to use positive conditional

88b54f8

biyeun reviewed Jul 9, 2024

View reviewed changes

nospame added 3 commits July 9, 2024 14:24

PR feedback: rename 'submitting mobile workers' to 'form-submitting m…

6d70f3f

…obile workers' everywhere

PR feedback: clarify comment and use if/else for readability

1233633

Submitting Mobile Worker => Form-Submitting Mobile Worker in migration

dc0aa82

nospame changed the title ~~Model changes for "submitting mobile workers" invoicing~~ Model changes for "form-submitting mobile workers" invoicing Jul 9, 2024

biyeun approved these changes Jul 10, 2024

View reviewed changes

nospame merged commit 10701a0 into master Jul 10, 2024
13 checks passed

nospame deleted the em/active-users-invoicing-model-changes branch July 10, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model changes for "form-submitting mobile workers" invoicing #34847

Model changes for "form-submitting mobile workers" invoicing #34847

nospame commented Jul 2, 2024 •

edited

Loading

dannyroberts Jul 9, 2024

nospame Jul 9, 2024

biyeun Jul 9, 2024

nospame Jul 9, 2024

dannyroberts Jul 9, 2024

nospame Jul 9, 2024

biyeun Jul 9, 2024

nospame Jul 9, 2024 •

edited

Loading

biyeun Jul 9, 2024

nospame Jul 9, 2024

biyeun Jul 9, 2024

nospame Jul 9, 2024

biyeun left a comment

biyeun Jul 9, 2024

biyeun Jul 9, 2024 •

edited

Loading

biyeun Jul 9, 2024

nospame Jul 9, 2024 •

edited

Loading

biyeun left a comment

Model changes for "form-submitting mobile workers" invoicing #34847

Model changes for "form-submitting mobile workers" invoicing #34847

Conversation

nospame commented Jul 2, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

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

nospame Jul 9, 2024 • edited Loading

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

biyeun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biyeun Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nospame Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

biyeun left a comment

Choose a reason for hiding this comment

nospame commented Jul 2, 2024 •

edited

Loading

nospame Jul 9, 2024 •

edited

Loading

biyeun Jul 9, 2024 •

edited

Loading

nospame Jul 9, 2024 •

edited

Loading