From b9ff81d59001fc81dbb7a5e1c0b4c07b49fd5517 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Thu, 25 Apr 2024 11:25:50 +0200 Subject: [PATCH 01/11] update task to run every minute --- corehq/apps/app_execution/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/app_execution/tasks.py b/corehq/apps/app_execution/tasks.py index 667231f2d029..45f50943a281 100644 --- a/corehq/apps/app_execution/tasks.py +++ b/corehq/apps/app_execution/tasks.py @@ -10,7 +10,7 @@ from corehq.util.log import send_HTML_email -@periodic_task(run_every=crontab(minute=0, hour=0)) +@periodic_task(run_every=crontab()) # run every minute def run_app_workflows(): for config in AppWorkflowConfig.objects.get_due(): From 943186f520707f1c159c6dc0d93312819f1c1b8c Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Thu, 25 Apr 2024 11:30:53 +0200 Subject: [PATCH 02/11] separate path for starting a session --- corehq/apps/app_execution/api.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/corehq/apps/app_execution/api.py b/corehq/apps/app_execution/api.py index c0802bea4566..8d704ebd51ae 100644 --- a/corehq/apps/app_execution/api.py +++ b/corehq/apps/app_execution/api.py @@ -292,6 +292,12 @@ def _get_base_data(self): "username": self.client.username, } + def start_session(self): + self.sync() + data = self.get_session_start_data() + self.data = self.client.make_request(data, "navigate_menu_start") + print("Starting app session:\n", file=self.log) + def execute_step(self, step): is_form_step = isinstance(step, (data_model.AnswerQuestionStep, data_model.SubmitFormStep)) if is_form_step and self.form_mode == const.FORM_MODE_IGNORE: @@ -300,13 +306,11 @@ def execute_step(self, step): if self.form_mode == const.FORM_MODE_NO_SUBMIT and isinstance(step, data_model.SubmitFormStep): self.log_step(step, skipped=True) return - data = self.get_request_data(step) if step else self.get_session_start_data() + data = self.get_request_data(step) self.data = self.client.make_request(data, self.request_url(step)) self.log_step(step) def log_step(self, step, indent=" ", skipped=False): - if not step: - print("Starting app session:\n", file=self.log) skipped_log = " (ignored)" if skipped else "" print(f"Execute step: {step or 'START'} {skipped_log}", file=self.log) if skipped: @@ -335,8 +339,7 @@ def log_step(self, step, indent=" ", skipped=False): def execute_workflow(session: FormplayerSession, workflow): with session: - session.sync() - execute_step(session, None) + session.start_session() for step in workflow.steps: execute_step(session, step) From 550a2f238c407dc15836c69e63e4c6b0fa856a6f Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Thu, 25 Apr 2024 11:32:37 +0200 Subject: [PATCH 03/11] add app version to log --- corehq/apps/app_execution/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/app_execution/api.py b/corehq/apps/app_execution/api.py index 8d704ebd51ae..e4c73af04a72 100644 --- a/corehq/apps/app_execution/api.py +++ b/corehq/apps/app_execution/api.py @@ -199,7 +199,7 @@ def app_build_id(self): 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) + print(f"Using app '{app.name}' ({app._id} - {app.version} - {build_on})", file=self.log) return app._id def sync(self): From fa577e439a2e7b8570c096093725329fb51211b6 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Thu, 25 Apr 2024 11:37:05 +0200 Subject: [PATCH 04/11] extract base builder class --- corehq/util/jsonattrs.py | 62 ++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/corehq/util/jsonattrs.py b/corehq/util/jsonattrs.py index b38184792dcc..9d08dc4bb287 100644 --- a/corehq/util/jsonattrs.py +++ b/corehq/util/jsonattrs.py @@ -196,15 +196,15 @@ def __init__(self, item_type, /, **jsonfield_args): @define -class AttrsListBuilder: +class BaseBuilder: attrs_type = field() - def attrify(self, items): + def attrify(self, value): attrs_type = self.attrs_type - if items is None: - return items + if value is None: + return value from_json = make_from_json(attrs_type) - return [from_json(item) for item in items] + return self._attrify(value, from_json) def jsonify(self, value): if not value: @@ -213,48 +213,42 @@ def jsonify(self, value): to_json = self.attrs_type.__jsonattrs_to_json__ else: to_json = asdict + return self._jsonify(value, to_json) + + def _attrify(self, value, from_json): + raise NotImplementedError() + + def _jsonify(self, value, to_json): + raise NotImplementedError() + + +@define +class AttrsListBuilder(BaseBuilder): + + def _attrify(self, value, from_json): + return [from_json(item) for item in value] + + def _jsonify(self, value, to_json): return [to_json(v) for v in value] @define -class AttrsDictBuilder: - attrs_type = field() +class AttrsDictBuilder(BaseBuilder): - def attrify(self, values): - attrs_type = self.attrs_type - if values is None: - return values - from_json = make_from_json(attrs_type) - return {key: from_json(value) for key, value in values.items()} + def _attrify(self, value, from_json): + return {key: from_json(value) for key, value in value.items()} - def jsonify(self, value): - if not value: - return value - if hasattr(self.attrs_type, "__jsonattrs_to_json__"): - to_json = self.attrs_type.__jsonattrs_to_json__ - else: - to_json = asdict + def _jsonify(self, value, to_json): return {k: to_json(v) for k, v in value.items()} @define -class AttrsObjectBuilder: - attrs_type = field() +class AttrsObjectBuilder(BaseBuilder): - def attrify(self, value): - attrs_type = self.attrs_type - if value is None: - return value - from_json = make_from_json(attrs_type) + def _attrify(self, value, from_json): return from_json(value) - def jsonify(self, value): - if not value: - return value - if hasattr(self.attrs_type, "__jsonattrs_to_json__"): - to_json = self.attrs_type.__jsonattrs_to_json__ - else: - to_json = asdict + def _jsonify(self, value, to_json): return to_json(value) From 4ce22d83e71068e100df93a09fd7fa728dfe75ec Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 14:03:17 +0200 Subject: [PATCH 05/11] don't modify data during wrapping --- corehq/apps/app_execution/data_model.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/corehq/apps/app_execution/data_model.py b/corehq/apps/app_execution/data_model.py index 3c3f9c16dfa2..98797d7501d8 100644 --- a/corehq/apps/app_execution/data_model.py +++ b/corehq/apps/app_execution/data_model.py @@ -1,5 +1,6 @@ from __future__ import annotations +import copy import dataclasses from typing import ClassVar @@ -210,6 +211,7 @@ def _append_selection(data, selection): def _steps_from_json(data): + data = copy.deepcopy(data) return [STEP_MAP[child.pop("type")].from_json(child) for child in data] From 5113e26c72a2c24e06864b55a3ed8121f24a0603 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 14:04:14 +0200 Subject: [PATCH 06/11] allow user to use username instead of user ID --- corehq/apps/app_execution/forms.py | 33 ++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/corehq/apps/app_execution/forms.py b/corehq/apps/app_execution/forms.py index 7b4baed3d1ac..227071318a6d 100644 --- a/corehq/apps/app_execution/forms.py +++ b/corehq/apps/app_execution/forms.py @@ -7,10 +7,13 @@ from corehq.apps.app_manager.dbaccessors import get_brief_app from corehq.apps.hqwebapp import crispy as hqcrispy from corehq.apps.users.models import CommCareUser +from corehq.apps.users.util import normalize_username class AppWorkflowConfigForm(forms.ModelForm): run_every = forms.IntegerField(min_value=1) + username = forms.CharField(max_length=255, label="Username", + help_text="Username of the user to run the workflow") class Meta: model = AppWorkflowConfig @@ -18,7 +21,6 @@ class Meta: "name", "domain", "app_id", - "user_id", "workflow", "sync_before_run", "form_mode", @@ -31,10 +33,20 @@ class Meta: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + if self.instance.id: + self.fields["username"].initial = self.instance.django_user.username self.helper = hqcrispy.HQFormHelper() self.helper.layout = crispy.Layout( - *self.fields.keys(), + "name", + "domain", + "app_id", + "username", + "workflow", + "sync_before_run", + "form_mode", + "run_every", + "notification_emails", hqcrispy.FormActions( twbscrispy.StrictButton("Save", type='submit', css_class='btn-primary') ), @@ -42,15 +54,24 @@ def __init__(self, *args, **kwargs): def clean(self): self.final_clean_app_id() - self.final_clean_user_id() + self.final_clean_username() return self.cleaned_data - def final_clean_user_id(self): + def final_clean_username(self): domain = self.cleaned_data.get("domain") + username = self.cleaned_data.get("username") + if username and "@" not in username: + try: + username = normalize_username(self.cleaned_data.get("username"), domain) + except ValueError: + self.add_error("username", "Invalid username") try: - self.commcare_user = CommCareUser.get_by_user_id(self.cleaned_data.get("user_id"), domain) + self.commcare_user = CommCareUser.get_by_username(username) except ResourceNotFound: - raise forms.ValidationError(f"User not found in domain: {domain}:{self.cleaned_data.get('user_id')}") + self.add_error("username", "User not found") + + if self.commcare_user.domain != domain: + self.add_error("username", f"User not found in domain: {domain}") def final_clean_app_id(self): domain = self.cleaned_data.get("domain") From 4f25928a11091d5bf764a178f407ba13ebd78c0d Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 14:04:59 +0200 Subject: [PATCH 07/11] remove unnecessary tag libs --- .../app_execution/templates/app_execution/workflow_form.html | 2 -- 1 file changed, 2 deletions(-) diff --git a/corehq/apps/app_execution/templates/app_execution/workflow_form.html b/corehq/apps/app_execution/templates/app_execution/workflow_form.html index 22cd3b46526a..d2995184db56 100644 --- a/corehq/apps/app_execution/templates/app_execution/workflow_form.html +++ b/corehq/apps/app_execution/templates/app_execution/workflow_form.html @@ -1,7 +1,5 @@ {% extends "hqwebapp/bootstrap5/base_section.html" %} {% load crispy_forms_tags %} -{% load hq_shared_tags %} -{% load i18n %} {% block page_content %} {% crispy form %} {% endblock %} From dd4b3331ec335c5d9a96e7611038ab52bd5eb4f8 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 14:05:06 +0200 Subject: [PATCH 08/11] update jsonattrs validation --- corehq/util/jsonattrs.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/corehq/util/jsonattrs.py b/corehq/util/jsonattrs.py index 9d08dc4bb287..00e060b86777 100644 --- a/corehq/util/jsonattrs.py +++ b/corehq/util/jsonattrs.py @@ -95,7 +95,7 @@ class Plane(models.Model): class JsonAttrsField(JSONField): default_error_messages = { - 'invalid': _("'%(field)s' field value has an invalid format: %(exc)s"), + 'invalid_attr': _("'%(field)s' field value has an invalid format: %(exc)s"), } def __init__(self, *args, builder, **kw): @@ -113,8 +113,8 @@ def to_python(self, value): return self.builder.attrify(value) except Exception as exc: raise ValidationError( - self.error_messages['invalid'], - code='invalid', + self.error_messages['invalid_attr'], + code='invalid_attr', params={ 'field': self.name, 'exc': BadValue.format(value, exc), @@ -341,13 +341,26 @@ def __name__(self): class JsonAttrsFormField(forms.JSONField): default_error_messages = { - 'invalid': _("'%(field)s' field value has an invalid format: %(exc)s"), + 'invalid_attr': _("%(exc)s"), } def __init__(self, builder, encoder=None, decoder=None, **kwargs): self.builder = builder super().__init__(encoder, decoder, **kwargs) + def validate(self, value): + super().validate(value) + try: + self.builder.attrify(value) + except Exception as exc: + raise ValidationError( + self.error_messages['invalid_attr'], + code='invalid_attr', + params={ + 'exc': BadValue.format(value, exc), + }, + ) + def prepare_value(self, value): if isinstance(value, str): return value From 614250a287140953a0e856c0e6e28995e55b3318 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 14:05:23 +0200 Subject: [PATCH 09/11] drop unique constraint --- ...2_alter_appworkflowconfig_unique_together.py | 17 +++++++++++++++++ corehq/apps/app_execution/models.py | 3 --- migrations.lock | 1 + 3 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 corehq/apps/app_execution/migrations/0002_alter_appworkflowconfig_unique_together.py diff --git a/corehq/apps/app_execution/migrations/0002_alter_appworkflowconfig_unique_together.py b/corehq/apps/app_execution/migrations/0002_alter_appworkflowconfig_unique_together.py new file mode 100644 index 000000000000..3bcb2ec51b1c --- /dev/null +++ b/corehq/apps/app_execution/migrations/0002_alter_appworkflowconfig_unique_together.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.25 on 2024-04-30 11:10 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('app_execution', '0001_initial'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='appworkflowconfig', + unique_together=set(), + ), + ] diff --git a/corehq/apps/app_execution/models.py b/corehq/apps/app_execution/models.py index d7fa493df5a0..fb1f9742c4d4 100644 --- a/corehq/apps/app_execution/models.py +++ b/corehq/apps/app_execution/models.py @@ -41,9 +41,6 @@ class AppWorkflowConfig(models.Model): objects = AppWorkflowManager() - class Meta: - unique_together = ("domain", "user_id") - @cached_property def app_name(self): app = get_brief_app(self.domain, self.app_id) diff --git a/migrations.lock b/migrations.lock index def6f4cf38b2..d3398f836dba 100644 --- a/migrations.lock +++ b/migrations.lock @@ -128,6 +128,7 @@ api 0004_rename_apiuser app_execution 0001_initial + 0002_alter_appworkflowconfig_unique_together app_manager 0001_linked_app_domain 0002_latestenabledbuildprofiles From e4d46b4f11492f1d8e426c5860a1de9912335360 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 14:11:52 +0200 Subject: [PATCH 10/11] also catch FormplayerException --- corehq/apps/app_execution/api.py | 6 +----- corehq/apps/app_execution/exceptions.py | 4 ++++ corehq/apps/app_execution/views.py | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/corehq/apps/app_execution/api.py b/corehq/apps/app_execution/api.py index e4c73af04a72..a79a2ba2bc98 100644 --- a/corehq/apps/app_execution/api.py +++ b/corehq/apps/app_execution/api.py @@ -13,7 +13,7 @@ from django.http import HttpRequest from corehq.apps.app_execution import const, data_model -from corehq.apps.app_execution.exceptions import AppExecutionError +from corehq.apps.app_execution.exceptions import AppExecutionError, FormplayerException from corehq.apps.app_manager.dbaccessors import get_app from corehq.apps.formplayer_api.sync_db import sync_db from corehq.apps.formplayer_api.utils import get_formplayer_url @@ -21,10 +21,6 @@ from dimagi.utils.web import get_url_base -class FormplayerException(Exception): - pass - - class BaseFormplayerClient: """Client class used to make requests to Formplayer""" diff --git a/corehq/apps/app_execution/exceptions.py b/corehq/apps/app_execution/exceptions.py index 6e8f3928b032..e6ed1b72cfb8 100644 --- a/corehq/apps/app_execution/exceptions.py +++ b/corehq/apps/app_execution/exceptions.py @@ -1,2 +1,6 @@ class AppExecutionError(Exception): pass + + +class FormplayerException(Exception): + pass diff --git a/corehq/apps/app_execution/views.py b/corehq/apps/app_execution/views.py index a753c9aee0ec..de29fbbd10f0 100644 --- a/corehq/apps/app_execution/views.py +++ b/corehq/apps/app_execution/views.py @@ -4,7 +4,7 @@ from corehq.apps.app_execution import const from corehq.apps.app_execution.api import execute_workflow from corehq.apps.app_execution.data_model import EXAMPLE_WORKFLOW -from corehq.apps.app_execution.exceptions import AppExecutionError +from corehq.apps.app_execution.exceptions import AppExecutionError, FormplayerException from corehq.apps.app_execution.forms import AppWorkflowConfigForm from corehq.apps.app_execution.models import AppWorkflowConfig from corehq.apps.domain.decorators import require_superuser_or_contractor @@ -73,7 +73,7 @@ def test_workflow(request, pk): session = config.get_formplayer_session() try: execute_workflow(session, config.workflow) - except AppExecutionError as e: + except (AppExecutionError, FormplayerException) as e: context["error"] = str(e) context["result"] = True From c392b179de50ebd46089319b30a3eab42054023d Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Tue, 30 Apr 2024 16:38:29 +0200 Subject: [PATCH 11/11] persist session ID and query data across requests --- corehq/apps/app_execution/api.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/corehq/apps/app_execution/api.py b/corehq/apps/app_execution/api.py index a79a2ba2bc98..14b65126167b 100644 --- a/corehq/apps/app_execution/api.py +++ b/corehq/apps/app_execution/api.py @@ -304,8 +304,15 @@ def execute_step(self, step): return data = self.get_request_data(step) self.data = self.client.make_request(data, self.request_url(step)) + self.augment_data_from_request(data, ["query_data", "session_id"]) self.log_step(step) + def augment_data_from_request(self, request_data, fields): + """Some fields aren't returned by Formplayer, so we need to maintain their value across requests""" + for field in fields: + if request_data.get(field) and not self.data.get(field): + self.data[field] = request_data[field] + def log_step(self, step, indent=" ", skipped=False): skipped_log = " (ignored)" if skipped else "" print(f"Execute step: {step or 'START'} {skipped_log}", file=self.log)