Automated application execution via formplayer by snopoke · Pull Request #34487 · dimagi/commcare-hq

snopoke · 2024-04-23T13:32:02Z

Technical Summary

This is a new admin feature which allows configuration of application workflows which will be executed on a schedule e.g. navigate through an app and complete a form.

The purpose of these is to generate 'canary' formplayer traffic for the purposes of monitoring and metrics.

It is expected that a small number of these will be configured per environment using a standardised application. There may also be additional configurations for monitoring enterprise apps.

Safety Assurance

Safety story

This is predominantly new functionality. There are some changes to existing code, notably jsonattrs which is used by repeaters. These changes are tested independently.

Automated test coverage

Some test coverage is added in this PR.

QA Plan

None

Migrations

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

Rollback instructions

This PR can be reverted after deploy with no further considerations

The DB migrations should be rolled back if the code is removed permanently.

Labels & Review

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

Local user auth allows us to authenticate without a password

This function also doesn't work since couch users don't have a 'django_user' field

snopoke · 2024-04-23T13:38:01Z

        return ret


-def django_user_from_couch_id(id):


This is unused and also broken since couch user docs don't contain a django_user field

snopoke · 2024-04-23T13:38:34Z

            'djangoUserId': user.pk,
            'superUser': user.is_superuser,
-            'authToken': None,
+            'authToken': session_id,


always return the session_id - this just makes it easier for formplayer. Since the session_id is passed as input there's no issue including it in the output.

dannyroberts

This looks incredible

dannyroberts · 2024-04-23T19:32:10Z

+from corehq.util.log import send_HTML_email
+
+
+@periodic_task(run_every=crontab(minute=0, hour=0))


Does this mean once a day?

esoergel

Haven't made it through the whole thing yet

esoergel · 2024-04-23T20:47:49Z

+    def _get_screen_and_data(self, current_data):
+        if not current_data:
+            return ScreenType.START, None
+        data = current_data.get("commands")


Is there any way to apply a schema or encode some expectations about the structure of current_data?

We could use dataclasses or attrs for each of the response types

esoergel · 2024-04-23T21:01:24Z

+        return factory.form_response(selections, self.children)
+
+
+APP1 = Menu(


esoergel · 2024-04-23T21:17:45Z

+            yield exploration
+            continue
+
+        exploration.execute_next_step()


What's with this stepwise execution? Looks like it's to go breadth-first rather than depth first, but it's evaluated fully either way, right? Just curious what that provides versus a recursive traversal.

ha, just saw commit # 2

esoergel · 2024-04-23T21:25:06Z

+def execute_workflow(session: FormplayerSession, workflow):
+    with session:
+        session.sync()
+        execute_step(session, None)


What would you think of moving self.get_session_start_data() here in place of None so that execute_step can assume step is always specified?

esoergel · 2024-04-23T21:35:53Z

    Endpoints with this decorator will not enforce two factor authentication.
    """
-    return get_multi_auth_decorator(default=BASIC, oauth_scopes=['sync'])(
+    return get_multi_auth_decorator(default=BASIC, allow_formplayer=True, oauth_scopes=['sync'])(


can you tell me more about this change? why did mobile_auth not previously allow formplayer auth? Was it that only smsforms needed it previously, and that doesn't need these endpoints?

I removed this and pushed it to #34441 in a different way: f16faf3

But to answer your question, there are some endpoints where we only expect mobile to auth and not formplayer so it makes sense to keep those separate. For endpoints that both access we can use the mobile_auth_or_formplayer decorator.

esoergel · 2024-04-23T22:00:23Z



+@define
+class AttrsObjectBuilder:


Is it the case that an AttrsList is a list of objects, and an AttrsDict is a dict whose values are objects, whereas this represents a single object? There are some repeated patterns between the three - would be nice to combine the builders reflecting the commonalities if that's doable.

Is it the case that an AttrsList is a list of objects, and an AttrsDict is a dict whose values are objects, whereas this represents a single object?

Yes, that's correct.

There are some repeated patterns between the three - would be nice to combine the builders reflecting the commonalities if that's doable.

I'll take a look

esoergel · 2024-04-23T22:03:08Z

    def execute_step(self, step):
        data = self.get_request_data(step) if step else self.get_session_start_data()
        self.data = self.client.make_request(data, self.request_url(step))
+        self.log_step(step)


this is nice

esoergel · 2024-04-23T22:06:28Z

    user_id = models.CharField(max_length=36)
    django_user = models.ForeignKey(User, on_delete=models.CASCADE)
    workflow = AttrsObject(AppWorkflow)
+    form_mode = models.CharField(max_length=255, choices=FORM_MODE_CHOICES)


FWIW models.TextChoices is a nice pattern for this sort of thing

esoergel · 2024-04-23T22:08:29Z

+    def final_clean_user_id(self):
+        domain = self.cleaned_data.get("domain")
+        try:
+            CommCareUser.get_by_user_id(domain, self.cleaned_data.get("user_id"))


Does it have to be a CommCareUser? USH is working to have web users submitting data too.

It doesn't have to be but for this use case I thought it made more sense for it to be a mobile user since they are easier to manage.

esoergel · 2024-04-23T22:22:02Z


+class AppWorkflowManager(models.Manager):
+    def get_due(self):
+        cutoff = functions.Now() - MakeInterval("mins", models.F("run_every"))


I've done this a few times now in different projects: https://www.snopoke.com/2023/03/11/django-intervals/

esoergel

Finished. There are some parts I could've gone more in depth reviewing, so please let me know if there's anything in particular you want a more detailed review

esoergel · 2024-04-23T23:37:35Z

+        build_on = "Latest Version"
+        if app.built_on:
+            build_on = app.built_on.strftime("%B %d, %Y")
+        print(f"Using app '{app.name}' ({app._id} - {build_on})", file=self.log)


Might be useful to log the app version number too - easier to cross reference on the releases page.

AmitPhulera · 2024-04-24T06:09:24Z

@esoergel Not sure if you have some blocking comments, maybe @snopoke can address them in a separate PR.
I have merged it for now to ensure that we have it up for testing today in EST. It felt safe to merge as it was a standalone thing and should not affect any part of HQ.

sentry · 2024-04-30T11:02:23Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ KeyError: 'field' /hq/admin/workflows/new/ View Issue
‼️ FormplayerException: Screen QueryScreen[ ] handling input ae76030e02e74264bc680f814ab86cc0 threw exception Index 1 ou... /hq/admin/workflows/test/{pk} View Issue

_{Did you find this useful? React with a 👍 or 👎}

snopoke added 30 commits April 12, 2024 13:51

initial app_execution framework

579c24e

convert to depth-first + cleanup

4db6252

test entity list selection

18dac1f

use correct url when in 'form' mode

77e5c61

use correct param name

b08233a

don't submit forms during discovery

fc0845b

basic execution test

4d79a70

refactor auth and implement session auth

020a3c1

implement query step for case search

7793fa6

implement serialization

e24a9b7

convert to jsonattrs

78390b8

minor refactor + implement local user auth

5b7c950

Local user auth allows us to authenticate without a password

add 'AttrsObject' field type to store a single object

be2132f

AppWorkflowConfig model

c5f976a

jsonattrs form field

bb02bb6

remove print

868af18

add an execution log

86f54fb

add form mode

2eb6b74

basic crud

28d7b0a

get django user from commcare user

0f40ba6

remove unused function

a1c520e

This function also doesn't work since couch users don't have a 'django_user' field

format json for view

871cf91

test view

a0593da

update to use bs5

04cac21

add schedule details and celery task

c9e233f

add notification emails

8fd9878

use latest released app version

6cdcd5e

sync before start

7520a4f

drop discovery

e3ea74e

revert change to auth decorator

42e72dd

snopoke added the product/invisible Change has no end-user visible impact label Apr 23, 2024

snopoke requested review from calellowitz and esoergel April 23, 2024 13:32

snopoke requested review from biyeun and orangejenny as code owners April 23, 2024 13:32

dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Apr 23, 2024

snopoke added 4 commits April 23, 2024 15:34

Merge branch 'master' into sk/app-execution

72e709b

revert auth decorator change property

66248f5

update tests

22bcadb

update session details test

827ffff

snopoke commented Apr 23, 2024

View reviewed changes

dannyroberts approved these changes Apr 23, 2024

View reviewed changes

dannyroberts reviewed Apr 23, 2024

View reviewed changes

esoergel reviewed Apr 23, 2024

View reviewed changes

AmitPhulera merged commit 5e46ccf into master Apr 24, 2024

snopoke deleted the sk/app-execution branch April 24, 2024 07:38

snopoke mentioned this pull request Apr 25, 2024

PR feedback from app execution changes #34501

Merged

3 tasks

snopoke mentioned this pull request Jun 11, 2026

Remove app_execution app #37822

Merged

4 tasks

		from corehq.util.log import send_HTML_email


		@periodic_task(run_every=crontab(minute=0, hour=0))

		return factory.form_response(selections, self.children)


		APP1 = Menu(



		@define
		class AttrsObjectBuilder:

Uh oh!

Uh oh!

Conversation

snopoke commented Apr 23, 2024

Technical Summary

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannyroberts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoergel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoergel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmitPhulera commented Apr 24, 2024

Uh oh!

sentry Bot commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suspect Issues

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sentry Bot commented Apr 30, 2024 •

edited

Loading