fix: confirm overwrite for ID verified fields

[michaelmwu]

Description

Added an explicit confirmation flow to mark-id-verified so non-blank cIdVerifiedBy and cIdVerifiedAt values are not overwritten accidentally.
Introduced MarkIdVerifiedOverwriteConfirmationView and updated _mark_id_verified_for_contact to read current verification fields, block direct overwrite when mismatched non-empty values exist, and persist only after explicit confirmation.
Single-contact and selection-based flows now share this guard through the same persistence path.
Updated unit tests in test_crm_mark_id_verified.py to validate GET-before-PUT behavior and the overwrite prompt flow.

Related Issue

• ID verification command should confirm if overwriting and changing non-blank verified by and verified at #66

How Has This Been Tested?

• uv run pytest tests/unit/test_crm_mark_id_verified.py (9 passed).

Summary by CodeRabbit

Release Notes

• New Features

  • Added a confirmation prompt when updating ID verification for contacts already marked as verified. Users must now explicitly approve before overwriting existing verification data.

• Tests

  • Expanded test coverage to validate the new verification overwrite confirmation workflow.

[coderabbitai]

Warning

Rate limit exceeded

@michaelmwu has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 58477df and 7f960c0.

📒 Files selected for processing (2)

• apps/discord_bot/src/five08/discord_bot/cogs/crm.py
• tests/unit/test_crm_mark_id_verified.py

📝 Walkthrough

Walkthrough

A new two-step confirmation flow for ID verification is added to the CRM cog. When verifying an already-verified contact, users receive a confirmation prompt before overwriting existing verification data. The _mark_id_verified_for_contact method now accepts an allow_overwrite parameter and includes logic to detect conflicts and trigger the confirmation UI.

Changes

Cohort / File(s)	Summary
ID Verification Overwrite Flow apps/discord_bot/src/five08/discord_bot/cogs/crm.py	Introduces MarkIdVerifiedOverwriteConfirmationView class for presenting a confirmation UI with Overwrite and Cancel buttons. Enhances _mark_id_verified_for_contact with allow_overwrite parameter; when conflicts are detected and overwrite is not allowed, the method audits the denial and shows the confirmation view; when allowed or no conflict exists, updates verification fields normally.
Test Coverage tests/unit/test_crm_mark_id_verified.py	Updates existing tests to simulate two-step API interaction (GET followed by PUT). Adds new test test_mark_id_verified_single_contact_prompts_for_overwrite_if_already_verified to verify confirmation view is triggered when attempting to re-verify an already-verified contact.

Sequence Diagram

sequenceDiagram
    participant User
    participant CRM Cog
    participant Espo API
    participant Confirmation View

    User->>CRM Cog: Request to verify contact ID
    CRM Cog->>Espo API: GET existing verification data
    Espo API-->>CRM Cog: Return current verifier/date
    alt Conflict detected & overwrite not allowed
        CRM Cog->>CRM Cog: Audit denial
        CRM Cog->>Confirmation View: Show overwrite confirmation
        Confirmation View->>User: Display Overwrite/Cancel buttons
        alt User clicks Overwrite
            User->>CRM Cog: Confirm overwrite
            CRM Cog->>Espo API: PUT with allow_overwrite=True
            Espo API-->>CRM Cog: Updated
            CRM Cog-->>User: Show success embed
        else User clicks Cancel
            User->>Confirmation View: Cancel
            Confirmation View-->>User: Hide UI
        end
    else No conflict or overwrite allowed
        CRM Cog->>Espo API: PUT verified_by/verified_at
        Espo API-->>CRM Cog: Updated
        CRM Cog-->>User: Show success embed
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Require id_type for mark-id-verified #76: Directly modifies the same _mark_id_verified_for_contact function and the ID verification flow in crm.py, indicating parallel or dependent work on verification logic.

Poem

🐰 A rabbit hops through Discord's halls,
"Verify twice!" the cog now calls,
No sneaky overwrites shall pass,
With confirmation, things have class! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: confirm overwrite for ID verified fields' clearly and concisely summarizes the main change: adding confirmation logic to prevent accidental overwrites of ID verification data.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch michaelmwu/id-verify-overwrite-check

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/unit/test_crm_mark_id_verified.py (1)

169-201: Add a partial-existing-values overwrite test case.

Please add coverage where only one existing field is non-blank (e.g., cIdVerifiedBy set, cIdVerifiedAt blank) and values differ, so the overwrite prompt behavior is pinned for that edge case too.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_crm_mark_id_verified.py` around lines 169 - 201, Add a new
async unit test mirroring
test_mark_id_verified_single_contact_prompts_for_overwrite_if_already_verified
that uses crm_cog._search_contacts_for_mark_id_verification to return the
contact and sets crm_cog.espo_api.request.return_value to have
ID_VERIFIED_BY_FIELD populated and ID_VERIFIED_AT_FIELD blank (or vice versa)
with differing values from the inputs; call crm_cog.mark_id_verified.callback
with the conflicting inputs and assert crm_cog.espo_api.request was called with
"GET" and the contact id, then assert mock_interaction.followup.send was invoked
with a message containing "already ID verified" and that
kwargs["view"].__class__ is MarkIdVerifiedOverwriteConfirmationView to confirm
the overwrite prompt appears for the partial-existing-values edge case.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 2452-2459: The overwrite-protection currently only triggers when
both existing_verified_by and existing_verified_at are non-empty; change the
logic in the CRM cog (where needs_confirmation is computed) to require
confirmation if either existing_verified_by or existing_verified_at is present
and the corresponding normalized value differs (i.e., check per-field: if
existing_verified_by is truthy and existing_verified_by !=
normalized_verified_by OR if existing_verified_at is truthy and
existing_verified_at != normalized_verified_at). Update the needs_confirmation
boolean to reflect this per-field conflict check so single-field conflicts
prompt confirmation.

---

Nitpick comments:
In `@tests/unit/test_crm_mark_id_verified.py`:
- Around line 169-201: Add a new async unit test mirroring
test_mark_id_verified_single_contact_prompts_for_overwrite_if_already_verified
that uses crm_cog._search_contacts_for_mark_id_verification to return the
contact and sets crm_cog.espo_api.request.return_value to have
ID_VERIFIED_BY_FIELD populated and ID_VERIFIED_AT_FIELD blank (or vice versa)
with differing values from the inputs; call crm_cog.mark_id_verified.callback
with the conflicting inputs and assert crm_cog.espo_api.request was called with
"GET" and the contact id, then assert mock_interaction.followup.send was invoked
with a message containing "already ID verified" and that
kwargs["view"].__class__ is MarkIdVerifiedOverwriteConfirmationView to confirm
the overwrite prompt appears for the partial-existing-values edge case.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec61b63 and 58477df.

📒 Files selected for processing (2)

• apps/discord_bot/src/five08/discord_bot/cogs/crm.py
• tests/unit/test_crm_mark_id_verified.py