feat(integration): Add health-tracking integration feature#7956
feat(integration): Add health-tracking integration feature#7956afsuyadi wants to merge 13 commits into
Conversation
…ration serializer
afsuyadi
requested review from
emyller and
talissoncosta
and removed request for
a team
|
@afsuyadi is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThis PR adds a generic integration health tracking feature via a new Estimated code review effort: 4 (Complex) | ~60 minutes Related PRsNone identified. Suggested labelsapi, frontend, integrations Suggested reviewersNone identified. Poem A rabbit checks each wire and call, Comment |
github-actions
Bot
added
front-end
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/integrations/sentry/change_tracking.py (1)
94-107: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftNetwork-level failures never produce a health record.
record_integration_healthis only reached on the success path (line 100), beforeraise_for_status(). Ifrequests.postitself raises (ConnectionError,Timeout, etc.), execution never reaches line 100, so no health record is written at all — the integration's health status silently goes stale instead of reflecting the outage.💡 Suggested direction
try: response = requests.post( url=self.webhook_url, headers=headers, data=json_payload, + timeout=10, ) record_integration_health(self.config, response.status_code) response.raise_for_status() # NOTE: This is for future-proofing, as Sentry won't respond 4xx. except requests.exceptions.RequestException as error: + record_integration_health(self.config, 0) log.warning( "request-failure", error=error, ) return
get_latest_integration_healthalready treats any non-2xxstatus_codeas unhealthy, so a sentinel value like0would correctly surface as "Unhealthy". This same gap likely applies to the other refactored wrappers (e.g.new_relic.py, which has no exception handling at all around its POST call).api/integrations/mixpanel/mixpanel.py (1)
22-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the health write behind best-effort handling.
record_integration_health(...)can raise, andidentify_user_async()does not catch it, so a DB hiccup here can fail the Mixpanel identify call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1890d82-3475-42d1-a441-d6f46c22fa78
📒 Files selected for processing (44)
api/app/settings/common.pyapi/audit/signals.pyapi/integrations/amplitude/amplitude.pyapi/integrations/amplitude/serializers.pyapi/integrations/common/apps.pyapi/integrations/common/migrations/0001_initial.pyapi/integrations/common/migrations/__init__.pyapi/integrations/common/models.pyapi/integrations/common/serializers.pyapi/integrations/common/services.pyapi/integrations/datadog/datadog.pyapi/integrations/dynatrace/dynatrace.pyapi/integrations/dynatrace/serializers.pyapi/integrations/grafana/grafana.pyapi/integrations/heap/heap.pyapi/integrations/heap/serializers.pyapi/integrations/mixpanel/mixpanel.pyapi/integrations/mixpanel/serializers.pyapi/integrations/new_relic/new_relic.pyapi/integrations/rudderstack/serializers.pyapi/integrations/segment/serializers.pyapi/integrations/sentry/change_tracking.pyapi/integrations/sentry/serializers.pyapi/integrations/webhook/serializers.pyapi/integrations/webhook/webhook.pyapi/tests/integration/sentry/test_change_tracking_webhook_integration.pyapi/tests/unit/integrations/amplitude/test_unit_amplitude.pyapi/tests/unit/integrations/amplitude/test_unit_amplitude_views.pyapi/tests/unit/integrations/common/test_unit_integrations_common_serializers.pyapi/tests/unit/integrations/common/test_unit_integrations_common_services.pyapi/tests/unit/integrations/datadog/test_unit_datadog.pyapi/tests/unit/integrations/dynatrace/test_unit_dynatrace.pyapi/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.pyapi/tests/unit/integrations/grafana/test_grafana.pyapi/tests/unit/integrations/heap/test_unit_heap.pyapi/tests/unit/integrations/heap/test_unit_heap_views.pyapi/tests/unit/integrations/mixpanel/test_unit_mixpanel.pyapi/tests/unit/integrations/mixpanel/test_unit_mixpanel_views.pyapi/tests/unit/integrations/new_relic/test_unit_new_relic.pyapi/tests/unit/integrations/rudderstack/test_unit_rudderstack_views.pyapi/tests/unit/integrations/segment/test_unit_segment_views.pyapi/tests/unit/integrations/webhook/test_unit_webhook.pyfrontend/common/types/responses.tsfrontend/web/components/IntegrationList.tsx
| self.config = config | ||
| self.api_key = config.api_key | ||
| self.url = f"{config.base_url}/identify" | ||
|
|
||
| def _identify_user(self, user_data: AmplitudeUserData) -> None: | ||
| payload = {"api_key": self.api_key, "identification": json.dumps([user_data])} | ||
|
|
||
| response = requests.post(self.url, data=payload) | ||
| record_integration_health(self.config, response.status_code) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the health helper, plus sibling integrations.
git ls-files api/integrations | sed -n '1,200p'
printf '\n--- amplitude.py ---\n'
cat -n api/integrations/amplitude/amplitude.py
printf '\n--- search record_integration_health usage ---\n'
rg -n "record_integration_health|logger.exception|try:|except Exception" api/integrations -S
printf '\n--- health helper definition ---\n'
rg -n "def record_integration_health|class .*IntegrationHealth|integration health" -S .
# If the helper is found, show its surrounding lines.
helper_file=$(rg -l "def record_integration_health" -S . | head -n 1 || true)
if [ -n "${helper_file:-}" ]; then
printf '\n--- helper file: %s ---\n' "$helper_file"
cat -n "$helper_file"
fi
# Show the sibling integration files that use the same pattern.
for f in api/integrations/heap/heap.py api/integrations/mixpanel/mixpanel.py api/integrations/webhook/webhook.py; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
doneRepository: Flagsmith/flagsmith
Length of output: 24975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- abstract wrapper ---'
cat -n api/integrations/common/wrapper.py
printf '\n%s\n' '--- search callers / exception handling around identify ---'
rg -n "identify_user|_identify_user|try:|except Exception|record_integration_health" api/integrations -S
printf '\n%s\n' '--- common services / model constraints ---'
cat -n api/integrations/common/models.pyRepository: Flagsmith/flagsmith
Length of output: 9356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- postpone decorator ---'
rg -n "def postpone|class .*postpone|`@postpone`" util api -S
postpone_file=$(rg -l "def postpone" util api -S | head -n 1 || true)
if [ -n "${postpone_file:-}" ]; then
printf '\n--- %s ---\n' "$postpone_file"
cat -n "$postpone_file"
fi
printf '\n%s\n' '--- integration entrypoint ---'
cat -n api/integrations/integration.py
printf '\n%s\n' '--- sentry change tracking pattern ---'
cat -n api/integrations/sentry/change_tracking.pyRepository: Flagsmith/flagsmith
Length of output: 493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- api/util/util.py ---'
cat -n api/util/util.py
printf '\n%s\n' '--- api/integrations/integration.py ---'
cat -n api/integrations/integration.py
printf '\n%s\n' '--- api/integrations/sentry/change_tracking.py ---'
cat -n api/integrations/sentry/change_tracking.pyRepository: Flagsmith/flagsmith
Length of output: 11465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ENABLE_POSTPONE_DECORATOR|postpone" -S api .env* settings* .Repository: Flagsmith/flagsmith
Length of output: 2101
Guard the health write in _identify_user. In synchronous mode (ENABLE_POSTPONE_DECORATOR=false), a DB or GenericForeignKey error here will bubble out of the Amplitude send; if health recording is meant to be best-effort, wrap it in a narrow try/except.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 28-28: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(self.url, data=payload)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[info] 28-28: no timeout was given on call to external resource
Context: requests.post(self.url, data=payload)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 26-26: use jsonify instead of json.dumps for JSON output
Context: json.dumps([user_data])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
| def _identify_user(self, user_data: AmplitudeUserData) -> None: | ||
| payload = {"api_key": self.api_key, "identification": json.dumps([user_data])} | ||
|
|
||
| response = requests.post(self.url, data=payload) | ||
| record_integration_health(self.config, response.status_code) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files | rg '^api/integrations/amplitude/|^api/.*config|^api/.*integration'
printf '\n== outline amplitude.py ==\n'
ast-grep outline api/integrations/amplitude/amplitude.py --view expanded || true
printf '\n== read amplitude.py ==\n'
wc -l api/integrations/amplitude/amplitude.py
cat -n api/integrations/amplitude/amplitude.py
printf '\n== search for base_url/url construction and timeout usage ==\n'
rg -n "base_url|timeout=|requests\.post\(|Amplitude" api/integrations -S
printf '\n== search for amplitude config definition ==\n'
rg -n "class .*Amplitude|base_url.*Amplitude|Amplitude.*Config|api_key" api -SRepository: Flagsmith/flagsmith
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== amplitude model ==\n'
cat -n api/integrations/amplitude/models.py
printf '\n== amplitude serializer ==\n'
cat -n api/integrations/amplitude/serializers.py
printf '\n== common environment integration model ==\n'
cat -n api/integrations/common/models.py
printf '\n== any URL validators on amplitude/base_url ==\n'
rg -n "AmplitudeConfiguration|base_url|URLField|validate|validator|private|internal|amplitude.com" api/integrations/amplitude api/integrations/common -S
printf '\n== any timeout constants or wrapper patterns in similar integrations ==\n'
rg -n "timeout=|requests\.post\(" api/integrations/{mixpanel,heap,amplitude,new_relic,dynatrace,grafana,datadog,sentry,lead_tracking} -SRepository: Flagsmith/flagsmith
Length of output: 8015
Add a timeout and restrict the Amplitude endpoint
requests.post() has no timeout, so a slow or unresponsive endpoint can hang the worker. base_url is stored and editable, then used directly to build the request URL, so it can point at arbitrary hosts; restrict it to Amplitude’s domain or remove the override.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 28-28: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(self.url, data=payload)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[info] 28-28: no timeout was given on call to external resource
Context: requests.post(self.url, data=payload)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 26-26: use jsonify instead of json.dumps for JSON output
Context: json.dumps([user_data])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
Source: Linters/SAST tools
| class IntegrationHealthRecord(models.Model): | ||
| created_at = models.DateTimeField("Created At", auto_now_add=True) | ||
| content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) | ||
| object_id = models.PositiveIntegerField() | ||
| content_object = GenericForeignKey("content_type", "object_id") | ||
| status_code = models.PositiveIntegerField() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider retention/cleanup for this append-only log.
Every event send inserts a new row (record_integration_health in api/integrations/common/services.py:7-14 always calls .create(...)) and only the latest row is ever read. With no periodic cleanup, this table will grow without bound proportional to integration traffic. Consider a scheduled cleanup task or capping history depth.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Add a composite index for the (content_type, object_id, created_at) lookup pattern.
get_latest_integration_health filters by content_type + object_id and orders by -created_at on every call. Without a supporting index, this degrades to a per-record-set scan as the table grows, since only content_type gets an implicit FK index.
💡 Proposed fix
class IntegrationHealthRecord(models.Model):
created_at = models.DateTimeField("Created At", auto_now_add=True)
content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE)
object_id = models.PositiveIntegerField()
content_object = GenericForeignKey("content_type", "object_id")
status_code = models.PositiveIntegerField()
+
+ class Meta:
+ indexes = [
+ models.Index(fields=["content_type", "object_id", "-created_at"]),
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class IntegrationHealthRecord(models.Model): | |
| created_at = models.DateTimeField("Created At", auto_now_add=True) | |
| content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) | |
| object_id = models.PositiveIntegerField() | |
| content_object = GenericForeignKey("content_type", "object_id") | |
| status_code = models.PositiveIntegerField() | |
| class IntegrationHealthRecord(models.Model): | |
| created_at = models.DateTimeField("Created At", auto_now_add=True) | |
| content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) | |
| object_id = models.PositiveIntegerField() | |
| content_object = GenericForeignKey("content_type", "object_id") | |
| status_code = models.PositiveIntegerField() | |
| class Meta: | |
| indexes = [ | |
| models.Index(fields=["content_type", "object_id", "-created_at"]), | |
| ] |
| def get_latest_health(self, obj): | ||
| return get_latest_integration_health(obj) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add type annotations to fix CI type-check failure.
Static analysis across Python 3.11/3.12/3.13 flags get_latest_health as missing a type annotation, which is causing pipeline failures.
🔧 Proposed fix
- def get_latest_health(self, obj):
+ def get_latest_health(self, obj: Model) -> dict | None:
return get_latest_integration_health(obj)(import Model from django.db.models if not already available.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_latest_health(self, obj): | |
| return get_latest_integration_health(obj) | |
| def get_latest_health(self, obj: Model) -> dict | None: | |
| return get_latest_integration_health(obj) |
🧰 Tools
🪛 GitHub Check: API Unit Tests (3.11)
[failure] 39-39:
Function is missing a type annotation
🪛 GitHub Check: API Unit Tests (3.12)
[failure] 39-39:
Function is missing a type annotation
🪛 GitHub Check: API Unit Tests (3.13)
[failure] 39-39:
Function is missing a type annotation
Source: Pipeline failures
| def record_integration_health( | ||
| integration_config: models.Model, | ||
| status_code: int, | ||
| ) -> None: | ||
| IntegrationHealthRecord.objects.create( | ||
| content_object=integration_config, | ||
| status_code=status_code, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider isolating health-write failures from the primary integration flow.
record_integration_health is invoked synchronously, unguarded, right after every wrapper's HTTP call (DataDog, Dynatrace, Grafana, New Relic, etc.). If this insert raises (DB contention, migration not yet applied in a rolling deploy, etc.), the exception propagates out of _track_event, and a health-bookkeeping failure would take down the primary event-tracking call path across every integration.
🛡️ Proposed defensive wrap
+import logging
+
+logger = logging.getLogger(__name__)
+
def record_integration_health(
integration_config: models.Model,
status_code: int,
) -> None:
- IntegrationHealthRecord.objects.create(
- content_object=integration_config,
- status_code=status_code,
- )
+ try:
+ IntegrationHealthRecord.objects.create(
+ content_object=integration_config,
+ status_code=status_code,
+ )
+ except Exception:
+ logger.exception("Failed to record integration health.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def record_integration_health( | |
| integration_config: models.Model, | |
| status_code: int, | |
| ) -> None: | |
| IntegrationHealthRecord.objects.create( | |
| content_object=integration_config, | |
| status_code=status_code, | |
| ) | |
| import logging | |
| logger = logging.getLogger(__name__) | |
| def record_integration_health( | |
| integration_config: models.Model, | |
| status_code: int, | |
| ) -> None: | |
| try: | |
| IntegrationHealthRecord.objects.create( | |
| content_object=integration_config, | |
| status_code=status_code, | |
| ) | |
| except Exception: | |
| logger.exception("Failed to record integration health.") |
| def test_base_environment_integration_model_serializer__latest_health__no_record( | ||
| environment, | ||
| ): | ||
| # Given | ||
| webhook_config = WebhookConfiguration.objects.create( | ||
| environment=environment, url="https://webhook.url" | ||
| ) | ||
|
|
||
| # When | ||
| serializer = WebhookConfigurationSerializer(webhook_config) | ||
| data = serializer.data | ||
|
|
||
| # Then | ||
| assert "latest_health" in data | ||
| assert data["latest_health"] is None | ||
|
|
||
|
|
||
| def test_base_environment_integration_model_serializer__latest_health__has_record( | ||
| environment, | ||
| ): | ||
| # Given | ||
| webhook_config = WebhookConfiguration.objects.create( | ||
| environment=environment, url="https://webhook.url" | ||
| ) | ||
| record_integration_health(webhook_config, 200) | ||
|
|
||
| # When | ||
| serializer = WebhookConfigurationSerializer(webhook_config) | ||
| data = serializer.data | ||
|
|
||
| # Then | ||
| assert data["latest_health"] is not None | ||
| assert data["latest_health"]["status_code"] == 200 | ||
| assert data["latest_health"]["is_healthy"] is True | ||
| assert data["latest_health"]["created_at"] is not None | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider also covering the unhealthy status case at the serializer level.
Both new tests cover "no record" and a healthy (200) record, but not a non-2xx status via the serializer (is_healthy: False). The boolean branch is exercised in the services tests, so this is a minor coverage gap rather than a defect.
| from integrations.webhook.models import WebhookConfiguration | ||
|
|
||
|
|
||
| def test_get_latest_integration_health__no_record__returns_none(environment): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add -> None type annotations to test functions.
Static analysis flags all four test functions as missing type annotations, failing the pipeline on 3.11/3.12/3.13.
🔧 Proposed fix
-def test_get_latest_integration_health__no_record__returns_none(environment):
+def test_get_latest_integration_health__no_record__returns_none(environment) -> None:(apply similarly to the other three test functions)
Also applies to: 21-21, 39-41, 57-57
🧰 Tools
🪛 GitHub Check: API Unit Tests (3.11)
[failure] 8-8:
Function is missing a type annotation
🪛 GitHub Check: API Unit Tests (3.12)
[failure] 8-8:
Function is missing a type annotation
🪛 GitHub Check: API Unit Tests (3.13)
[failure] 8-8:
Function is missing a type annotation
Source: Pipeline failures
| record_integration_health(webhook_config, 500) | ||
| record_integration_health(webhook_config, 200) | ||
|
|
||
| # When | ||
| result = get_latest_integration_health(webhook_config) | ||
|
|
||
| # Then | ||
| assert result["status_code"] == 200 | ||
| assert result["is_healthy"] is True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow result before indexing to satisfy type checker.
get_latest_integration_health returns dict | None, so indexing result["status_code"]/result["is_healthy"] at lines 53-54 and 68 fails static analysis (flagged across all three Python versions). Line 32 already asserts result is not None before indexing; the other two tests should do the same.
🔧 Proposed fix
result = get_latest_integration_health(webhook_config)
# Then
+ assert result is not None
assert result["status_code"] == 200
assert result["is_healthy"] is True(apply the same assert result is not None before indexing in the non-healthy-status test)
Also applies to: 62-69
🧰 Tools
🪛 GitHub Check: API Unit Tests (3.11)
[failure] 54-54:
Value of type "dict[Any, Any] | None" is not indexable
[failure] 53-53:
Value of type "dict[Any, Any] | None" is not indexable
🪛 GitHub Check: API Unit Tests (3.12)
[failure] 54-54:
Value of type "dict[Any, Any] | None" is not indexable
[failure] 53-53:
Value of type "dict[Any, Any] | None" is not indexable
🪛 GitHub Check: API Unit Tests (3.13)
[failure] 54-54:
Value of type "dict[Any, Any] | None" is not indexable
[failure] 53-53:
Value of type "dict[Any, Any] | None" is not indexable
Source: Pipeline failures
| def test_webhook_identify_user__records_health_status( # type: ignore[no-untyped-def] | ||
| mocker, | ||
| integration_webhook_config, | ||
| ): | ||
| # Given | ||
| webhook_wrapper = WebhookWrapper(integration_webhook_config) | ||
| response = mocker.MagicMock(status_code=200) | ||
| mocker.patch( | ||
| "integrations.webhook.webhook.call_integration_webhook", | ||
| return_value=response, | ||
| ) | ||
|
|
||
| # When | ||
| webhook_wrapper._identify_user({"identity": "identity-1"}) | ||
|
|
||
| # Then | ||
| health_record = IntegrationHealthRecord.objects.get( | ||
| object_id=integration_webhook_config.id | ||
| ) | ||
| assert health_record.status_code == 200 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test doesn't cover the failure path, and masks the webhook.py truthiness bug.
This test only exercises status_code=200. Because response is a mocker.MagicMock(status_code=200), it's truthy regardless of the configured status_code (MagicMock doesn't emulate requests.Response.__bool__), so even a mock configured with status_code=500 would still pass if response: in webhook.py — this test can't detect the bug flagged there. Add a negative-path test using a mock that reflects real Response truthiness (e.g. mocker.Mock(spec=requests.Response, status_code=500, __bool__=lambda self: False)), analogous to the amplitude healthy/unhealthy pair.
| <div className='d-flex align-items-center gap-2 mt-1 mr-3'> | ||
| <BooleanDotIndicator | ||
| enabled={integration.latest_health?.is_healthy ?? false} | ||
| /> | ||
| <span className='fs-small text-muted'> | ||
| {integration.latest_health | ||
| ? integration.latest_health.is_healthy | ||
| ? 'Healthy' | ||
| : 'Unhealthy' | ||
| : 'No health data'} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Nested ternary flagged by lint; extract for readability.
Static analysis flags the nested ternary at Line 350 (Do not nest ternary expressions). Extract the status text into a small helper/variable for clarity and to satisfy lint.
♻️ Suggested refactor
+ const healthStatusText = !integration.latest_health
+ ? 'No health data'
+ : integration.latest_health.is_healthy
+ ? 'Healthy'
+ : 'Unhealthy'
+
<div className='d-flex align-items-center gap-2 mt-1 mr-3'>
<BooleanDotIndicator
enabled={integration.latest_health?.is_healthy ?? false}
/>
<span className='fs-small text-muted'>
- {integration.latest_health
- ? integration.latest_health.is_healthy
- ? 'Healthy'
- : 'Unhealthy'
- : 'No health data'}
+ {healthStatusText}
</span>
</div>🧰 Tools
🪛 GitHub Check: Lint changed files
[failure] 350-350:
Do not nest ternary expressions
Source: Pipeline failures
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #4324
What I understand and how I build this feature:
Users need health tracker for every thirdy-party integration tool that will indicate whether it has showed healthy / unhealthy responses.
We need a specific table (
IntegrationHealthRecord) that holds the information about which third-party tool we integrate with and its health status.I create 2 helper functions:
record_integration_health(): create records in the specific table, if the response is2xx.get_latest_integration_health(): verify if the latest record on a thirdyparty config is presentWhen our server sends payload through a wrapper, that third-party tool will send back a response. That response is what we capture and stores in the special table. I don't include the data payload of the response, to prevent the bulking of database's size.
For every third-party tool, a health indicator on the client will appear with 3 status:
Healthy(2xxresponse),Unhealthy(anything that excludes from 2xx response), andNo Health Data(indicates that integrations hasn't happened, but the user has add that config with base url)For additional information:
latest_healthfield. To mitigate this, I also create unit tests to cover this.How did you test this code?
I create several unit tests that cover various flow:
latest_healthpresent and shaped correctly in the JSON response?)record_integration_healthafter the HTTP call?)CLI:
.venv/bin/pytest --ds=app.settings.test tests/unit/integrations/ -n0Result of testing all of the file:
After changes:
