Update Mobile UCR Versions for Domains (Script) #35158

zandre-eng · 2024-10-01T09:34:30Z

Product Description

No user-facing changes.

Technical Summary

Link to ticket here.

Please note: This PR should not be merged and is only created for review purposes. The script contained in this PR will be run as a once-off script.

This PR is the code for a once-off script that will be run to update the Mobile UCR versions of all domains from V1 to V2. This will only be done for domains that do not have any manual references to V1 fixtures. Domains that do contain V1 fixtures will be flagged and will need to be handled separately.

Feature Flag

MOBILE_UCR

Safety Assurance

Safety story

Local testing

Automated test coverage

N/A

QA Plan

No QA planned.

Labels & Review

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

kaapstorm

Just three suggestions, nothing blocking. I am happy with your approach.

corehq/mobile_ucr_v2_update_script.py

gherceg · 2024-10-01T10:34:11Z

Excited to see this happening! I see there are some tickets (here and here) already created to default to V2, but I can't tell if those changes have been deployed/what the current behavior on production is. It would make sense to me to ensure we default to V2 prior to running this, so just making sure that is the plan or if not, there is a reason why.

zandre-eng · 2024-10-01T10:39:19Z

Thanks for sharing the concern @gherceg! I definitely agree this would be important to do beforehand and can confirm that we do have a plan outlined to first ensure that domains aren't able to go back to V1 before running the migration script from this PR. This also includes the related code changes from this Jira ticket to handle when users are reverting/publishing apps with V1 references.

mkangia

Non blocking comments.

Would be nice to have a -dry-run option to the script so it can be run without doing actual updates to see which apps it would update or not update eventually. I have always found that useful, specifically on production

mkangia · 2024-10-03T11:03:04Z

corehq/mobile_ucr_v2_update_script.py

+
+    mobile_ucr_domains = find_domains_with_toggle_enabled(MOBILE_UCR)
+
+    log_file = open(LOG_FILE, 'a')


I believe this would leave the connection to the file open even after the script exits abruptly, so we should this with with option. This way you also don't need to worry about explicitly calling close.
with open(LOG_FILE, 'a') as log_file:

Does mode 'a' create the file if its missing?

Looking through the code, I can remove the need for passing the log file around to the various helper functions. Since we already know what the file path is, we can just use this directly in the helper function for adding to the log. Addressed in 27c1c25.

Does mode 'a' create the file if its missing?

It does. Here is an article for more info on the append mode.

mkangia · 2024-10-03T11:04:43Z

corehq/mobile_ucr_v2_update_script.py

+        apps = get_apps_by_id(domain, app_ids)
+        for app in apps:
+            try:
+                # Don't look at app.is_released since the latest version might not be released yet


Confirming that we are just picking app and not releases.
I believe one way to confirm that is by checking copy_of property on the application. Is it blank for applications and not blank for releases. it would be a good check to have though I assume you are anyway only picking up applications and not releases.

Confirming that we are just picking app and not releases.

Could you please elaborate on what you mean by app vs release? Are you referring to app versions that have been marked as "Released" or are you referring to the app versions themselves?

This comment refers that we are picking the latest version of each app and we don't want to consider app.is_released since the latest version of an app might still be set to "Test" and not "Released"

Are you referring to app versions that have been marked as "Released" or are you referring to the app versions themselves?

app versions or app builds.

We are getting the applications themselves using get_apps_by_id() which returns a list of Application objects. Since we are only getting the latest release of each app, these retrieved application objects should be the current build of the app.

@mkangia I'm not sure if the above answers your question, but let me know if it doesn't and I'd be happy to offline on this.

Since we are only getting the latest release of each app, these retrieved application objects should be the current build of the app.

I think there is a misunderstanding of how apps work.

So, an application is actually a running doc and the versions are saved instances of that application at different points(versions). My understanding is that here we want to update the application and not the latest created version/build.

Seems like we are okay here since you are using get_latest_app_ids_and_versions which is using app_manager/applications_brief couch view which only keeps applications and not builds since it only saves docs that have copy_of set as null

mkangia · 2024-10-03T11:08:54Z

corehq/mobile_ucr_v2_update_script.py

+V1_FIXTURE_IDENTIFIER = 'src="jr://fixture/commcare:reports'
+V1_FIXTURE_PATTERN = r'<.*src="jr://fixture/commcare:reports.*>'
+V1_REFERENCES_PATTERN = r"<.*instance\('reports'\)/reports/.*>"
+RE_V1_ALL_REFERENCES = re.compile(f"{V1_FIXTURE_PATTERN}|{V1_REFERENCES_PATTERN}")


I trust this has been tested so it matches as needed.

These regex patterns come from this script which has been used to analyze all the domains with V1 references. Based on the results of the script that was run, these regex patterns have successfully picked up domains with V1 references.

These references were taken from this Confluence page, which shows what needs to change to migrate from V1->V2.

mkangia · 2024-10-03T11:11:46Z

corehq/mobile_ucr_v2_update_script.py

+skip_domains = set()
+
+
+def process():


nit: the flow of code in the file made it a bit hard to follow along. Just reminding on keeping things in a file in logical order. I am big fan on vertical formatting

I can see how this might be difficult with all the helper functions at the top. I've refactored this to have the main process function at the top instead eac2bcf.

zandre-eng · 2024-10-03T13:46:08Z

Would be nice to have a -dry-run option to the script so it can be run without doing actual updates to see which apps it would update or not update eventually.

@mkangia I think this is a great idea. I have added a dry run option in 20d9401.

mkangia · 2024-10-07T11:08:00Z

corehq/mobile_ucr_v2_update_script.py

@@ -37,7 +37,7 @@
 skip_domains = set()


-def process():
+def process(dry_run=False):


nit: totally up to you and fine to leave it as it is for this one, I usually keep dry run as True by default, it avoids accidental updates for me, but it is also fine either way.

That sounds like a good idea. Addressed in 25a0298.

corehq/mobile_ucr_v2_update_script.py

ajeety4

Looks good to me.
Just a non-blocking comment.

Also I think it would be good to test that after updating the app to v2 (through script or manually), it does not break anything. Preferably one of the app from echis domain.

corehq/mobile_ucr_v2_update_script.py

ajeety4 · 2024-10-15T14:31:49Z

corehq/mobile_ucr_v2_update_script.py

+                # Don't look at app.is_released since the latest version might not be released yet
+                if app.mobile_ucr_restore_version != '2.0':
+                    save_in_log(f"Processing App: {domain}: {app.name}: {app.id}")
+                    if not has_non_v2_form(domain, app):


Based on this

Suggested change

if not has_non_v2_form(domain, app):

if not has_non_v2_reference(domain, app):

Addressed in 6dcc719.

corehq/mobile_ucr_v2_update_script.py

ajeety4

Requested some changes based on latest findings as explained here

ajeety4 · 2024-10-15T14:55:42Z

Hey @zandre-eng ,

Wanted to add one more thing based on earlier script run for finding non-v2 apps.
The django machine for India is quite small (2 GB) and as a result, this script will cause memory error.
So we will need to do some slight modifications before running this for India. The modifications I made are in this gist. I can offline with you on this.

zandre-eng · 2024-10-16T08:12:38Z

Wanted to add one more thing based on earlier script run for finding non-v2 apps.
The django machine for India is quite small (2 GB) and as a result, this script will cause memory error.
So we will need to do some slight modifications before running this for India. The modifications I made are in this gist. I can offline with you on this.

@ajeety4 Ah this is good to know, thanks for flagging. I have made a few adjustments based on the gist you provided (241fd96). Let me know if it looks good, or if there was something else I missed. Thanks!

ajeety4 · 2024-10-16T14:35:37Z

Wanted to add one more thing based on earlier script run for finding non-v2 apps.
The django machine for India is quite small (2 GB) and as a result, this script will cause memory error.
So we will need to do some slight modifications before running this for India. The modifications I made are in this gist. I can offline with you on this.

@ajeety4 Ah this is good to know, thanks for flagging. I have made a few adjustments based on the gist you provided (241fd96). Let me know if it looks good, or if there was something else I missed. Thanks!

Hey @zandre-eng , the changes looks good. Thanks for adding this. FYI for the run, I had set the memory limit to 900 MB.

One more adjustment I had to make to get the successful run for some of the domains was below, basically fetching one app at at time. Could be done now or while running the script.

Instead of below,

        apps = get_apps_by_id(domain, app_ids)
        for app in apps:

I used this

        for app_id in app_ids:
            app = get_apps_by_id(domain, [app_id])[0]

zandre-eng · 2024-10-17T07:07:12Z

FYI for the run, I had set the memory limit to 900 MB.

That's good to know, thanks!

One more adjustment I had to make to get the successful run for some of the domains was below, basically fetching one app at at time. Could be done now or while running the script.

@ajeety4 Was this specifically for the India environment? Doing it in this manner will incur additional DB hits since we'll know need to retrieve each app one at a time. I think if this is specific to only the India env then I'll only let this adjustment run for them and keep the current functionality for the production env. This will help to minimize the amount of DB calls for envs that have a larger working memory.

ajeety4 · 2024-10-17T07:37:35Z

FYI for the run, I had set the memory limit to 900 MB.

That's good to know, thanks!

One more adjustment I had to make to get the successful run for some of the domains was below, basically fetching one app at at time. Could be done now or while running the script.

@ajeety4 Was this specifically for the India environment? Doing it in this manner will incur additional DB hits since we'll know need to retrieve each app one at a time. I think if this is specific to only the India env then I'll only let this adjustment run for them and keep the current functionality for the production env. This will help to minimize the amount of DB calls for envs that have a larger working memory.

Yes specific to the India environment. I agree with keeping the current code for the production environment and doing the adjustment at the run time sounds good.

zandre-eng · 2024-10-17T08:12:59Z

Yes specific to the India environment. I agree with keeping the current code for the production environment and doing the adjustment at the run time sounds good.

@ajeety4 I decided to be more explicit about it and implement code for both workflows so that it's more clear what needs to change for low memory envs (0833b6f).

zandre-eng added 2 commits October 1, 2024 11:26

implement script

5184652

logging for apps containing v1 refs

30a6a13

zandre-eng added the product/invisible Change has no end-user visible impact label Oct 1, 2024

zandre-eng requested review from kaapstorm, mkangia, Charl1996 and ajeety4 October 1, 2024 09:34

zandre-eng added the Open for review: do not merge A work in progress label Oct 1, 2024

kaapstorm approved these changes Oct 1, 2024

View reviewed changes

corehq/mobile_ucr_v2_update_script.py Outdated Show resolved Hide resolved

corehq/mobile_ucr_v2_update_script.py Outdated Show resolved Hide resolved

corehq/mobile_ucr_v2_update_script.py Outdated Show resolved Hide resolved

zandre-eng added 2 commits October 1, 2024 12:15

compile re expression

0392d0f

use sets instead of lists

b3e4252

kaapstorm approved these changes Oct 1, 2024

View reviewed changes

mkangia approved these changes Oct 3, 2024

View reviewed changes

mkangia reviewed Oct 3, 2024

View reviewed changes

zandre-eng added 3 commits October 3, 2024 15:14

refactor logging

27c1c25

reorganize script flow

eac2bcf

add option for dry run

20d9401

mkangia reviewed Oct 7, 2024

View reviewed changes

mkangia approved these changes Oct 7, 2024

View reviewed changes

ajeety4 reviewed Oct 9, 2024

View reviewed changes

corehq/mobile_ucr_v2_update_script.py Show resolved Hide resolved

ajeety4 approved these changes Oct 9, 2024

View reviewed changes

ajeety4 reviewed Oct 15, 2024

View reviewed changes

corehq/mobile_ucr_v2_update_script.py Show resolved Hide resolved

ajeety4 reviewed Oct 15, 2024

View reviewed changes

corehq/mobile_ucr_v2_update_script.py Outdated Show resolved Hide resolved

ajeety4 reviewed Oct 15, 2024

View reviewed changes

corehq/mobile_ucr_v2_update_script.py Outdated Show resolved Hide resolved

ajeety4 requested changes Oct 15, 2024

View reviewed changes

zandre-eng added 4 commits October 16, 2024 10:03

default to doing dry run

25a0298

optimizations for low memory env

241fd96

check for v1 refs in case list/detail section

6dcc719

skip app if no reporting modules

f86eeef

ajeety4 approved these changes Oct 16, 2024

View reviewed changes

zandre-eng added 2 commits October 17, 2024 10:08

support single app retrieval

0833b6f

small refactor

9de34f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Mobile UCR Versions for Domains (Script) #35158

Update Mobile UCR Versions for Domains (Script) #35158

zandre-eng commented Oct 1, 2024

kaapstorm left a comment

gherceg commented Oct 1, 2024

zandre-eng commented Oct 1, 2024

mkangia left a comment

mkangia Oct 3, 2024

zandre-eng Oct 3, 2024

mkangia Oct 3, 2024

zandre-eng Oct 3, 2024

mkangia Oct 7, 2024

zandre-eng Oct 16, 2024

mkangia Oct 16, 2024

mkangia Oct 3, 2024

zandre-eng Oct 3, 2024

mkangia Oct 3, 2024

zandre-eng Oct 3, 2024

zandre-eng commented Oct 3, 2024

mkangia Oct 7, 2024

zandre-eng Oct 16, 2024

ajeety4 left a comment

ajeety4 Oct 15, 2024

zandre-eng Oct 16, 2024

ajeety4 left a comment

ajeety4 commented Oct 15, 2024

zandre-eng commented Oct 16, 2024

ajeety4 commented Oct 16, 2024

zandre-eng commented Oct 17, 2024

ajeety4 commented Oct 17, 2024

zandre-eng commented Oct 17, 2024


		mobile_ucr_domains = find_domains_with_toggle_enabled(MOBILE_UCR)

		log_file = open(LOG_FILE, 'a')

	if not has_non_v2_form(domain, app):
	if not has_non_v2_reference(domain, app):

Update Mobile UCR Versions for Domains (Script) #35158

Are you sure you want to change the base?

Update Mobile UCR Versions for Domains (Script) #35158

Conversation

zandre-eng commented Oct 1, 2024

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Labels & Review

kaapstorm left a comment

Choose a reason for hiding this comment

gherceg commented Oct 1, 2024

zandre-eng commented Oct 1, 2024

mkangia 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

zandre-eng commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajeety4 left a comment

Choose a reason for hiding this comment

ajeety4 commented Oct 15, 2024

zandre-eng commented Oct 16, 2024

ajeety4 commented Oct 16, 2024

zandre-eng commented Oct 17, 2024

ajeety4 commented Oct 17, 2024

zandre-eng commented Oct 17, 2024