Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for custom callouts on app builds #35628

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Check for custom callouts on app builds #35628

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a847070
Add method for checking custom callouts
Charl1996 Jan 16, 2025
dd8145e
Tag app id
Charl1996 Jan 16, 2025
9e400b1
Add task for managing metrics for app build
Charl1996 Jan 17, 2025
16745fb
Invoke task after build has saved
Charl1996 Jan 17, 2025
fb0ff78
Make sure to fetch the apps deterministically
Charl1996 Jan 17, 2025
2741b7e
Remove pickle serialization
Charl1996 Jan 17, 2025
265a39d
Fix test
Charl1996 Jan 17, 2025
30598b0
Merge branch 'master' into cs/SC-4113-check-for-custom-callouts
Charl1996 Jan 20, 2025
f274054
Pass build id instead of app id
Charl1996 Jan 20, 2025
8b8e04c
Pull out function to calculate ids and memoize result
Charl1996 Jan 20, 2025
cd95688
Merge branch 'master' into cs/SC-4113-check-for-custom-callouts
Charl1996 Jan 20, 2025
452b577
Merge branch 'cs/SC-4113-check-for-custom-callouts' of github.com:dim…
Charl1996 Jan 20, 2025
6968738
Replace memoized -> quickcache
Charl1996 Jan 21, 2025
58df9e1
Merge branch 'master' into cs/SC-4113-check-for-custom-callouts
Charl1996 Jan 21, 2025
751d406
Merge branch 'master' into cs/SC-4113-check-for-custom-callouts
Charl1996 Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions corehq/apps/app_manager/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@
from corehq.blobs.mixin import CODES, BlobMixin
from corehq.const import USER_DATE_FORMAT, USER_TIME_FORMAT
from corehq.util import bitly, view_utils
from corehq.util.metrics import metrics_counter
from corehq.util.quickcache import quickcache
from corehq.util.timer import TimingContext, time_method
from corehq.util.timezones.conversions import ServerTime
Expand Down Expand Up @@ -4468,31 +4467,8 @@ def make_build(self, comment=None, user_id=None):
assert copy._id
prune_auto_generated_builds.delay(self.domain, self._id)

self.check_build_dependencies(new_build=copy)

return copy

def check_build_dependencies(self, new_build):
"""
Reports whether the app dependencies have been added or removed.
"""

def has_dependencies(build):
return bool(
build.profile.get('features', {}).get('dependencies')
)

new_build_has_dependencies = has_dependencies(new_build)

last_build = get_latest_build_doc(self.domain, self.id)
last_build = self.__class__.wrap(last_build) if last_build else None
last_build_has_dependencies = has_dependencies(last_build) if last_build else False

if not last_build_has_dependencies and new_build_has_dependencies:
metrics_counter('commcare.app_build.dependencies_added')
elif last_build_has_dependencies and not new_build_has_dependencies:
metrics_counter('commcare.app_build.dependencies_removed')

def convert_app_to_build(self, copy_of, user_id, comment=None):
self.copy_of = copy_of
built_on = datetime.datetime.utcnow()
Expand Down
52 changes: 52 additions & 0 deletions corehq/apps/app_manager/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
get_app,
get_auto_generated_built_apps,
get_latest_build_id,
get_build_ids,
)
from corehq.apps.app_manager.exceptions import (
AppValidationError,
Expand All @@ -15,6 +16,7 @@
from corehq.apps.users.models import CommCareUser, CouchUser
from corehq.toggles import USH_USERCASES_FOR_WEB_USERS
from corehq.util.decorators import serial_task
from corehq.util.metrics import metrics_counter

logger = get_task_logger(__name__)

Expand Down Expand Up @@ -91,3 +93,53 @@ def update_linked_app_and_notify_task(domain, app_id, master_app_id, user_id, em
def load_appcues_template_app(domain, username, app_slug):
from corehq.apps.app_manager.views.apps import load_app_from_slug
load_app_from_slug(domain, username, app_slug)


@task(queue='background_queue', ignore_result=True)
def analyse_new_app_build(domain, new_build_id):
new_build = get_app(domain, new_build_id)

check_for_custom_callouts(new_build)
check_build_dependencies(new_build)


def check_for_custom_callouts(new_build):
from corehq.apps.app_manager.util import app_callout_templates_ids

def app_has_custom_intents():
return any(
any(set(form.wrapped_xform().odk_intents) - app_callout_templates_ids)
for form in new_build.get_forms()
)

if app_has_custom_intents():
metrics_counter(
'commcare.app_build.custom_app_callout',
tags={'domain': new_build.domain, 'app_id': new_build.copy_of},
)


def check_build_dependencies(new_build):
"""
Reports whether the app dependencies have been added or removed.
"""

def has_dependencies(build):
return bool(
build.profile.get('features', {}).get('dependencies')
)

new_build_has_dependencies = has_dependencies(new_build)
app_build_ids = get_build_ids(new_build.domain, new_build.copy_of)

last_build_has_dependencies = False

if len(app_build_ids) > 1:
previous_build_id = app_build_ids[app_build_ids.index(new_build.id) + 1]
previous_build = get_app(new_build.domain, previous_build_id)
Comment on lines +138 to +139
Copy link

@coderabbitai coderabbitai bot Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential IndexError when accessing previous build ID

In check_build_dependencies, lines 142-143 calculate previous_build_id using app_build_ids[app_build_ids.index(new_build.id) + 1]. If new_build.id is the last element in app_build_ids, this will result in an IndexError. Additionally, if new_build.id is not in app_build_ids, a ValueError will be raised.

Modify the code to handle these edge cases safely:

    if len(app_build_ids) > 1:
        try:
            idx = app_build_ids.index(new_build.id)
            if idx + 1 < len(app_build_ids):
                previous_build_id = app_build_ids[idx + 1]
                previous_build = get_app(new_build.domain, previous_build_id)
                last_build_has_dependencies = has_dependencies(previous_build) if previous_build else False
        except (ValueError, IndexError):
            # Handle the case where new_build.id is not found or is the last element
            last_build_has_dependencies = False

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Charl1996 Would you like me to help you implement the suggested fix to handle the edge cases safely? Let me know if you need any assistance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's fine. I don't think this will be a concern because the docstring in the get_build_ids says the following:
"Returns all the built apps for an application id, in descending order of time built."

I've verified that it's indeed so, meaning

  1. the new_build.id will definitely exist in the list
  2. if it's the last item in the list it also means its the only item in the list and the len(app_build_ids) will evaluate to false, meaning this block of code won't execute in the first place.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Charl1996 I've found the implementation in dbaccessors.py and can confirm your explanation. The docstring indeed explicitly states that it "Returns all the built apps for an application id, in descending order of time built." This validates your assessment of the code's safety. Thank you for helping me understand the implementation better.

🔗 Analysis chain

@Charl1996 Thank you for the detailed explanation. I understand now that the code is indeed safe due to the guarantees provided by get_build_ids. Your explanation makes perfect sense - since the builds are returned in descending order and the new build will always be present, we don't need additional error handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the implementation of get_build_ids
ast-grep --pattern 'def get_build_ids'

# Also search for any documentation or comments about get_build_ids
rg -A 5 'def get_build_ids'

Length of output: 1548

✏️ Learnings added 
Learnt from: Charl1996
PR: dimagi/commcare-hq#35628
File: corehq/apps/app_manager/tasks.py:142-143
Timestamp: 2025-01-20T08:22:44.938Z
Learning: The `get_build_ids` function in corehq.apps.app_manager.dbaccessors returns all built apps for an application ID in descending order of build time, which guarantees that:
1. A newly built app's ID will always exist in the returned list
2. If the new build ID is the last item in the list, it means it's the only item, preventing any IndexError in the `check_build_dependencies` function

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a carrot, Mr Rabbit!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Charl1996 🥕 Thank you for the carrot! happy rabbit noises 🐰

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this shouldn't be an issue, but you could save readers from going through the same thought process you outlined, and make it obvious that isn't an issue, by doing something like:

build_index = app_build_ids.index(new_build.id)
if build_index < len(app_build_ids):
    previous_build_id = app_build_ids[build_index + 1]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

last_build_has_dependencies = has_dependencies(previous_build) if previous_build else False

if not last_build_has_dependencies and new_build_has_dependencies:
metrics_counter('commcare.app_build.dependencies_added')
elif last_build_has_dependencies and not new_build_has_dependencies:
metrics_counter('commcare.app_build.dependencies_removed')
13 changes: 7 additions & 6 deletions corehq/apps/app_manager/tests/test_apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from corehq.apps.domain.shortcuts import create_domain
from corehq.apps.linked_domain.applications import link_app
from corehq.apps.userreports.tests.utils import get_sample_report_config
from corehq.apps.app_manager.views.releases import make_app_build


MockRequest = namedtuple('MockRequest', ['status', 'data'])
Expand Down Expand Up @@ -423,8 +424,8 @@ def test_dependencies_feature_added(self):
factory.app.save()
self.addCleanup(factory.app.delete)

with patch("corehq.apps.app_manager.models.metrics_counter") as metric_counter_mock:
factory.app.make_build()
with patch("corehq.apps.app_manager.tasks.metrics_counter") as metric_counter_mock:
make_app_build(factory.app, "comment", user_id="user_id")
Charl1996 marked this conversation as resolved.
Show resolved Hide resolved
metric_counter_mock.assert_called_with('commcare.app_build.dependencies_added')

def test_dependencies_feature_removed(self):
Expand All @@ -440,8 +441,8 @@ def test_dependencies_feature_removed(self):
factory.app.profile = {'features': {'dependencies': []}}
factory.app.save()

with patch("corehq.apps.app_manager.models.metrics_counter") as metric_counter_mock:
factory.app.make_build()
with patch("corehq.apps.app_manager.tasks.metrics_counter") as metric_counter_mock:
make_app_build(factory.app, "comment", user_id="user_id")
metric_counter_mock.assert_called_with('commcare.app_build.dependencies_removed')

def test_dependencies_feature_metrics_not_triggerd(self):
Expand All @@ -456,6 +457,6 @@ def test_dependencies_feature_metrics_not_triggerd(self):

factory.app.save()

with patch("corehq.apps.app_manager.models.metrics_counter") as metric_counter_mock:
factory.app.make_build()
with patch("corehq.apps.app_manager.tasks.metrics_counter") as metric_counter_mock:
make_app_build(factory.app, "comment", user_id="user_id")
metric_counter_mock.assert_not_called()
7 changes: 7 additions & 0 deletions corehq/apps/app_manager/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from couchdbkit import ResourceNotFound
from couchdbkit.exceptions import DocTypeError

from memoized import memoized
from dimagi.utils.couch import CriticalSection

from corehq import toggles
Expand Down Expand Up @@ -540,7 +541,13 @@ def _app_callout_templates():
yield data


@memoized
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memoized is only an in-memory cache, so it isn't the thing to use here, you'll want to cache across requests. I was thinking you could quickcache, probably for a while, maybe a week or longer. Check out these docs if you haven't seen them.

Copy link
Contributor Author

@Charl1996 Charl1996 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memoized is only an in-memory cache ... you'll want to cache across requests

Ah, I thought it was the other way around (but haven't read that docs to be honest). Probably the "quick" in quickcache that got my mind on the wrong track :shrug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6968738

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks for updating

def _app_callout_templates_ids():
return {t['id'] for t in next(_app_callout_templates())}


app_callout_templates = _app_callout_templates()
app_callout_templates_ids = _app_callout_templates_ids()


def purge_report_from_mobile_ucr(report_config):
Expand Down
2 changes: 2 additions & 0 deletions corehq/apps/app_manager/views/releases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
)
from corehq.apps.app_manager.tasks import (
create_build_files_for_all_app_profiles,
analyse_new_app_build,
)
from corehq.apps.app_manager.util import (
get_and_assert_practice_user_in_domain,
Expand Down Expand Up @@ -387,6 +388,7 @@ def make_app_build(app, comment, user_id):
user_id=user_id,
)
copy.save(increment_version=False)
analyse_new_app_build.delay(app.domain, copy._id)
return copy


Expand Down