Add annual software plans as default product plans #34584
Conversation
- Break down into more model-specific _ensure functions - namedtuple for plan_key parameters - get_or_create for SoftwarePlan - helpers for plan name and SoftwareProduct - kwargs dict for SoftwarePlan and DefaultProductPlan
Adding is_annual_plan to unique_together constraint means the model can support a monthly and an annual version of what otherwise looks like the same plan
dimagimon
added
the
reindex/migration
nospame
added
the
product/all-users-all-environments
nospame
requested review from
millerdev and
biyeun
and removed request for
biyeun
No need to pass in all params when name is sufficient. Reverts to previous behavior where visibility and edition are silently ignored if they mismatch the existing plan.
|
|
||
| def _software_plan_name(plan_key, product, product_rate): | ||
| plan_name = ( | ||
| (('%s Trial' % product_rate.name) if plan_key.is_trial else ('%s Edition' % product_rate.name)) |
There was a problem hiding this comment.
I know this is copied from very old code, but can we use f-strings to make this more readable?
| (('%s Trial' % product_rate.name) if plan_key.is_trial else ('%s Edition' % product_rate.name)) | |
| (f'{product_rate.name} Trial' if plan_key.is_trial else f'{product_rate.name} Edition') |
There was a problem hiding this comment.
Could this need to be translated, or is this always used for internal representations like log_accounting_info? If it may be translated then f-strings cannot be used, although string.format() is still better than the old % syntax in most cases.
There was a problem hiding this comment.
As far as I can see it's only ever used internally, since we're using it to look up and/or generate model instances here, or in other places for log_accounting_info. Since the instance name does get shown to a user in limited cases like renewing their plan, it could be that it's translated later on (I don't think so, but it could be), but I would think that would be based on the name saved on the instance, not this string construction.
| @@ -25,47 +26,47 @@ def tearDown(self): | |||
| def test_ensure_plans(self): | |||
| self._test_plan_versions_ensured(BOOTSTRAP_CONFIG_TESTING) | |||
| self._test_plan_versions_ensured({ | |||
| (SoftwarePlanEdition.COMMUNITY, False, False): { | |||
| (SoftwarePlanEdition.COMMUNITY, False, False, False): { | |||
There was a problem hiding this comment.
were any tests added for the annual plans?
There was a problem hiding this comment.
nm I see in BOOTSTRAP_CONFIG_TESTING those annual plans are added
Also remove duplicate logging for software product rate, and note whether default product plan is an annual plan.
|
Want to confirm there are no particular concerns with this. Don't know that I've ever not been able to check that box before.
|
|
@millerdev Hi Daniel, any thoughts on this? |
There was a problem hiding this comment.
The migration here relies on code changes to ensure_plans to run correctly.
Thanks for bringing this to my attention. I didn't notice it and did not plan to do a thorough review of this PR since Biyeun had already reviewed it.
With regard to:
The migrations in this code can be safely applied first independently of the code
Another way to say this is, some currently-deployed code would be broken and cause errors if the migrations in this PR were to be applied before the PR is deployed. (This is what happens, for a usually short time, during every deploy.)
It's not clear to me (yet) if that is true for this PR, but it would be true, for example, in a PR that deleted a table and also removed code that referenced that same table. Those two things (remove code that references to-be-deleted table and migration that deletes a table) need to be done in separate PRs (and deployed probably 6 weeks apart) so that there is no live code that references the table by the time the migration is applied. The 6-week constraint is so self-hosted environments have a chance to apply the migration before the breaking change is deployed.
On the other hand, it's fine for a single PR to include a migration that adds a new table and also include code that references that new table. This is safe since there is no deployed code that references the table.
Does this explanation help you form a better answer? Hopefully it was helpful and not a bunch of things you already knew.
| @@ -857,7 +857,7 @@ class DefaultProductPlan(models.Model): | |||
|
|
|||
| class Meta(object): | |||
| app_label = 'accounting' | |||
| unique_together = ('edition', 'is_trial', 'is_report_builder_enabled') | |||
| unique_together = ('edition', 'is_trial', 'is_report_builder_enabled', 'is_annual_plan') | |||
There was a problem hiding this comment.
I'm not sure this can be done safely in a single operation. Looking at the output of sqlmigrate, it will do the following inside a transaction:
ALTER TABLE "accounting_defaultproductplan" DROP CONSTRAINT "accounting_defaultproduc_edition_is_trial_is_repo_294e5a62_uniq";
ALTER TABLE "accounting_defaultproductplan" ADD CONSTRAINT "accounting_defaultproduc_edition_is_trial_is_repo_46637956_uniq" UNIQUE ("edition", "is_trial", "is_report_builder_enabled", "is_annual_plan");I'm not sure how much data is in that table, but if there is a lot and it takes a long time, it will lock the table while the new index is being built. This means that all other queries will be blocked during that time.
An alternate and safer approach would be to have the migration create the new index first CONCURRENTLY, and then after that is finished, drop the old index. Note that creating an index concurrently cannot be done in a transaction, so the migration will need to specify atomic = False. Search for other migrations where we have run CREATE UNIQUE INDEX CONCURRENTLY for examples.
This question and answer seem relevant since you are altering a constraint, but I think only indexes (unique or not) can be created concurrently.
Finally, are there any potential uniqueness violations in the existing data? If so, this will fail if they are not removed before this is run.
There was a problem hiding this comment.
Another way to say this is, some currently-deployed code would be broken and cause errors if the migrations in this PR were to be applied before the PR is deployed.
...
Does this explanation help you form a better answer?
Yes, I think so. I believe this is closer to "include a migration that adds a new table and also include code that references that new table", in that the migration first alters a table, and then relies on that altered table state to add new software plans.
I'm not sure how much data is in that table
Quite little - essentially one object for every uniquely named Software Plan, in this case a quick query returns a total of 12 DefaultProductPlans on production, 10 on staging, and 15 (after running this migration) in my local environment.
I'm not really familiar with indices. Would creating the new index CONCURRENTLY first mean we're generating a new set of data with the change applied, waiting for that to complete, and then pointing the db to the new data? It seems like this might not be necessary given the small volume of data, but is it better practice to just go this route anyway?
Finally, are there any potential uniqueness violations in the existing data?
There are not. DefaultProductPlan is only created in ensure_plans, which itself is only run in migrations. The is_annual_plan property was only recently added, with a default of False that is now applied to all instances existing prior to this PR, and because of the current constraint could not violate the new one.
There was a problem hiding this comment.
If there are so few (i.e., less than thousands) it will be no problem to do it this way, no need to do it CONCURRENTLY. This assumes that there is not high contention for locks on that table, which I assume there are not since it's probably rarely written, given the small number of rows.
| @@ -9,6 +9,15 @@ def _bootstrap_new_annual_pricing(apps, schema_editor): | |||
| ensure_plans(BOOTSTRAP_CONFIG, verbose=True, apps=apps) | |||
|
|
|||
|
|
|||
| def _remove_annual_default_product_plans(apps, schema_editor): | |||
There was a problem hiding this comment.
I don't know if this is strictly necessary but was easy enough to add, so that in case it needs to be reverted for any reason we won't be stuck doing so manually on every environment.
There was a problem hiding this comment.
👍 It's unlikely that we would use this in production environments once it's been merged into master because it's difficult to make sure all affected environments have been properly reversed. The approach we would likely take there would be to add a new migration that undoes whatever was done here that didn't work out. However, it can be very handy to be able to reverse migrations for development purposes.
nospame
added
product/admin
| product_rate.name = product_name | ||
|
|
||
| def _get_software_product(product_name, verbose, apps): | ||
| # SoftwareProduct no longer exists but is retained here to avoid breaking old migrations |
There was a problem hiding this comment.
Not sure I follow what's going on here, but if this is just to support very old migrations (like much older than 6 weeks) I would suggest to remove it and squash the migrations that depend on it so traces of SoftwareProduct can be removed. Fine if this is done in a separate PR.
There was a problem hiding this comment.
I fully agree, but I'm not actually sure how to do that. SoftwareProduct is currently created in accounting migration 0001_squashed_0052, modified in 0010 and finally removed in 0011, all of which are ~7 years old. In the meantime, ensure_plans gets run in accounting migrations 0001_squashed and 0008 so it has to account for the SoftwareProduct model existing.
The similarly aged comment that stood here previously noted that this could be removed "after squashing migrations" -- I'm not sure if there was more squashing intended or if it was just not quite correct.
| @@ -9,6 +9,15 @@ def _bootstrap_new_annual_pricing(apps, schema_editor): | |||
| ensure_plans(BOOTSTRAP_CONFIG, verbose=True, apps=apps) | |||
|
|
|||
|
|
|||
| def _remove_annual_default_product_plans(apps, schema_editor): | |||
There was a problem hiding this comment.
👍 It's unlikely that we would use this in production environments once it's been merged into master because it's difficult to make sure all affected environments have been properly reversed. The approach we would likely take there would be to add a new migration that undoes whatever was done here that didn't work out. However, it can be very handy to be able to reverse migrations for development purposes.
Product Description
Adds "Pay Annually" software plans if they do not already exist, using the naming convention "CommCare {edition} Edition - Pay Annually".
Technical Summary
SaaS-15493
Review by commit recommended.
Feature Flag
This change is not feature flagged.
Safety Assurance
Safety story
Tested migration directly in local environment and on staging, confirming new plans have been created or modified as expected and not duplicated.
Because
ensure_plansis always run via migrations, these changes are tested implicitly every time db test setup runs.Automated test coverage
Added config for annual plans and new asserts in
test_ensure_plans.QA Plan
Given complexity I ran into locally in constructing these changes, I would consider QA here, but I am not sure how to do so since it is primarily a data migration. Any thoughts?
Migrations
The migration here relies on code changes to
ensure_plansincluded in this PR to run correctly.Rollback instructions
The migration should be reversed first. This would remove
DefaultProductPlanobjects added here but not anySoftwarePlans, because on some environments those annual plans would have already existed.Labels & Review