[OSDEV-1827] SLC. Email - Incorrect text after approving the contribution to an existing production location that has been claimed or has a pending claim request

[roman-stolar]

OSDEV-1827 SLC. Email - Incorrect text after approving the contribution to an existing production location that has been claimed or has a pending claim request

• Fixed condition logic for email template when approving the contribution to an existing production location that has been claimed or has a pending claim request
• Removed duplicate method

[barecheck]

React App | Jest test suite - Code coverage report

Total: 32.85%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[vladsha-dev]

LGTM

[VadimKovalenkoSNF]

LGTM

[coderabbitai]

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/react/src/__tests__/components/ClaimedFacilitiesList.test.js (2)

44-44: Add missing semicolon for consistency.

The closing parenthesis of the beforeEach block is missing a semicolon, which should be added for consistency with the coding style in the rest of the file.

-    })
+    });

---

46-55: Consider adding test cases for scenarios with claimed facilities.

The test suite only covers the scenario where no claimed production locations are available. Consider adding test cases for:

1. When claimed facilities exist (checking proper rendering of the facilities list)
2. When facilities are being fetched (loading state)
3. Error handling scenarios

This would ensure the component behaves correctly across all possible states.

🛑 Comments failed to post (1)

src/react/src/__tests__/components/ClaimedFacilitiesList.test.js (1)

49-54: 💡 Verification agent

🧩 Analysis chain

Potential accessibility confusion with button role and href attribute.

The test is checking for an element with role="button" that also has an href attribute. This suggests the component might be using an anchor tag styled as a button. For better accessibility:

1. If this is meant to be a navigation element, consider using getByRole('link') instead of getByRole('button') in the test
2. If it's meant to be a button that triggers navigation, ensure the component is properly implementing ARIA attributes

---

🏁 Script executed:

#!/bin/bash
# Check the actual implementation of the ClaimedFacilitiesList component
cat src/react/src/components/ClaimedFacilitiesList.jsx | grep -A 10 "Find My Production Location"

Length of output: 323

---

Accessibility Concern: Update Role to Match Navigation Behavior

The test currently queries for an element with a button role even though the rendered component appears to be a navigation element (it has an href attribute). Consider one of the following adjustments:

• If this element is intended for navigation:

  • Update the test to use getByRole('link', { name: /Find My Production Location/i }) instead of getByRole('button').

• If it must remain a button triggering navigation:

  • Ensure the component explicitly implements appropriate ARIA attributes to clarify its interactive behavior.

These changes will help align the component’s semantics with its behavior and avoid potential accessibility confusion.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🧹 Nitpick comments (2)

deployment/terraform/anonymized_database_dump_scheduled_task/docker/dump.sh (2)

16-17: Nitpick: Variable Scope for ATTEMPT and MAX_ATTEMPT
The variables ATTEMPT and MAX_ATTEMPT are exported but are only used within this script. If there is no requirement for these variables to be available in child processes, consider declaring them as regular shell variables (i.e., remove the export) to limit their scope.

---

20-20: Address Shellcheck Warning SC2155: Separate Assignment and Export
Line 20 currently declares and exports SNAPSHOT_STATUS in one command. This can mask the return value of the command substitution. Consider separating the assignment from the export. For example:

-    export SNAPSHOT_STATUS=$(aws rds describe-db-snapshots --db-snapshot-identifier $ANONYMIZED_SNAPSHOT_ID --query 'DBSnapshots[0].Status' --output text)
+    SNAPSHOT_STATUS=$(aws rds describe-db-snapshots --db-snapshot-identifier $ANONYMIZED_SNAPSHOT_ID --query 'DBSnapshots[0].Status' --output text)
+    export SNAPSHOT_STATUS

This approach avoids masking potential errors during the command execution.

🧰 Tools

🪛 Shellcheck (0.10.0)

[warning] 20-20: Declare and assign separately to avoid masking return values.

(SC2155)

🛑 Comments failed to post (1)

deployment/terraform/anonymized_database_dump_scheduled_task/docker/dump.sh (1)

28-35: 💡 Verification agent

❓ Verification inconclusive

Review Timeout Loop Logic for Potential Off-by-One Concerns
Within the loop, the condition if [[ $ATTEMPT -le $MAX_ATTEMPT ]]; allows the script to sleep (and increment ATTEMPT) even when ATTEMPT equals MAX_ATTEMPT before eventually timing out. This may introduce an off-by-one behavior. Consider restructuring the loop to increment ATTEMPT at the beginning or check the condition before sleeping to ensure the script exits exactly when the maximum allowed attempts are reached. For instance, an alternative structure could be:

-    if [[ $ATTEMPT -le $MAX_ATTEMPT ]];
-    then
-        sleep 60s
-    else
-        echo "Snapshot creation timed out"
-        exit 1
-    fi
-    ATTEMPT=$((ATTEMPT + 1))
+    ATTEMPT=$((ATTEMPT + 1))
+    if [[ $ATTEMPT -gt $MAX_ATTEMPT ]]; then
+        echo "Snapshot creation timed out"
+        exit 1
+    fi
+    sleep 60s

This change checks the attempt count immediately after incrementing and exits once the maximum attempt limit is exceeded.

---

Attention: Off-by-One in the Timeout Loop

The current loop logic increments the attempt counter after the sleep interval, which means that when ATTEMPT equals MAX_ATTEMPT, the script still pauses before eventually timing out. To ensure the script exits as soon as the maximum attempts are exceeded, consider incrementing ATTEMPT at the beginning of the loop and checking the condition immediately. For example:

-    if [[ $ATTEMPT -le $MAX_ATTEMPT ]];
-    then
-        sleep 60s
-    else
-        echo "Snapshot creation timed out"
-        exit 1
-    fi
-    ATTEMPT=$((ATTEMPT + 1))
+    ATTEMPT=$((ATTEMPT + 1))
+    if [[ $ATTEMPT -gt $MAX_ATTEMPT ]]; then
+        echo "Snapshot creation timed out"
+        exit 1
+    fi
+    sleep 60s

This change ensures that the timeout condition is evaluated immediately after incrementing the counter, avoiding any off-by-one behavior.

📝 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.

    ATTEMPT=$((ATTEMPT + 1))
    if [[ $ATTEMPT -gt $MAX_ATTEMPT ]]; then
        echo "Snapshot creation timed out"
        exit 1
    fi
    sleep 60s

[VadimKovalenkoSNF]

Approved.