feat: integrate profile completeness check into captcha verification#9
Open
rezhajulio wants to merge 4 commits into
Open
feat: integrate profile completeness check into captcha verification#9rezhajulio wants to merge 4 commits into
rezhajulio wants to merge 4 commits into
Conversation
- Update captcha welcome message to inform users about profile requirements - Add profile completeness check (photo + username) in captcha callback handler - Reject verification with alert if profile is incomplete, keeping timeout active - Fix double query.answer() bug — each code path now answers exactly once - Add CAPTCHA_INCOMPLETE_PROFILE_MESSAGE constant for rejection alert - Update and add tests for new captcha+profile flow (553 tests, 99% coverage)
- Add parse guard for malformed callback data (ValueError/IndexError) - Move timeout job cancellation after successful unrestrict + DB cleanup - Wrap DB finalization in try/except to prevent unanswered callbacks - Replace AsyncMock with ProfileCheckResult dataclass in all test mocks - Add tests for malformed callback data and unrestrict failure preserving timeout
Apply review feedback to feat/captcha-profile-check. The previous order (unrestrict first, then DB cleanup) left Telegram state ahead of DB state on a DB write failure: user unrestricted in Telegram, pending captcha still in DB, timeout still armed. When the timeout fired, handle_captcha_expiration re-interpreted the inconsistency as a timeout event, sending contradictory success+timeout messages and a false-positive "unrestricted" DM notification. Reorder so the irreversible Telegram side effect happens LAST: - DB finalization (remove_pending_captcha + start_new_user_probation) first. Reversible. If it fails, user stays restricted, DB row preserved, timeout armed, user can retry. - unrestrict_user second. Irreversible. If it fails, DB row is gone but user is still restricted on Telegram; verify button is gone; user waits for admin. State is consistent. Tests - Replace hard-coded "Gagal memverifikasi..." literal with CAPTCHA_FAILED_VERIFICATION_MESSAGE constant in two tests (test_unknown_group_in_callback_rejects, test_malformed_callback_data_rejected) to remove drift risk. - Rename test_unrestrict_failure_stops_execution -> test_unrestrict_failure_keeps_user_restricted and invert assertions for the new order (DB row is now None, edit_message_text NOT called). - Add test_db_finalization_failure_keeps_user_restricted covering the DB-cleanup-failure path: unrestrict_user NOT called, DB row preserved, query shows failure alert. - Delete redundant test_profile_check_exception_shows_error (strict subset of test_captcha_callback_profile_check_exception). - Delete test_unrestrict_failure_preserves_timeout_job (its assertion was wrong for the new order; the renamed test covers the intent).
rezhajulio
force-pushed
the
feat/captcha-profile-check
branch
from
37a3c8c to
8c429e3
Compare
Round-1 follow-ups on feat/captcha-profile-check. All items are mechanical cleanups; no behavior change. Constants - Add CAPTCHA_PROFILE_CHECK_FAILED_MESSAGE. The previous behavior conflated "Telegram API failed" with "user failed verification" by showing the same generic CAPTCHA_FAILED_VERIFICATION_MESSAGE for both. The new message tells the user to retry in a few seconds, which is the right UX for a transient API error. Handler - captcha_callback_handler uses CAPTCHA_PROFILE_CHECK_FAILED_MESSAGE in the check_user_profile exception path. Other failure modes (parse error, missing pending captcha, unrestrict failure, DB finalization failure) still use CAPTCHA_FAILED_VERIFICATION_MESSAGE because those are deterministic verification failures, not API blips. Tests - Add _make_callback_query helper to TestCaptchaCallbackHandler. Refactor 10 tests to use it instead of repeating the 7-line MagicMock setup. Net diff: -55 lines. - Parametrize test_captcha_callback_incomplete_profile_rejected across (photo-only-missing, both-missing, username-only-missing) using missing_items as the parameter and computing the expected CAPTCHA_INCOMPLETE_PROFILE_MESSAGE via MISSING_ITEMS_SEPARATOR. The username-only-missing case is new coverage. - Parametrize test_malformed_callback_data_rejected across 4 variants: non-numeric single token, missing parts (IndexError), int parse failure (ValueError), unparseable second part. Drop the empty-string case (CallbackQueryHandler's regex filters it before the handler sees it). - Strengthen test_incomplete_profile_blocks_verification: replace "Lengkapi" + "username" substring assertions with a full-string match against CAPTCHA_INCOMPLETE_PROFILE_MESSAGE.format(missing_text="username"). - Add edit_message_text.assert_not_called() to the parametrized malformed test (Fix 5).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combines captcha verification with a mandatory profile completeness check (photo + username) before unrestricting new members.
Changes
Captcha + Profile Check Integration
Handler Hardening
Test Improvements
AsyncMock(is_complete=...)with realProfileCheckResultdataclass in all mocksFlow
Tests: 555 passed, ~99.9% coverage