feat: Add is_expired property for verifications #121

CuriousLearner · 2025-11-20T18:00:20Z

No description provided.

Copilot

Pull Request Overview

This PR adds expiration checking functionality to phone verifications through a new is_expired property and renames the expiration setting from SECURITY_CODE_EXPIRATION_TIME to the more explicit SECURITY_CODE_EXPIRATION_SECONDS. The changes maintain full backward compatibility with the old setting name while issuing deprecation warnings. Additionally, a new management command enables automated cleanup of old verification records, and the Django admin interface is enhanced with expiration status indicators.

Key Changes

Setting Rename: Deprecated SECURITY_CODE_EXPIRATION_TIME in favor of SECURITY_CODE_EXPIRATION_SECONDS with backward compatibility and deprecation warnings
Model Enhancement: Added is_expired property to SMSVerification model for easy expiration checking without manual datetime comparisons
Database Cleanup: New cleanup_phone_verifications management command with configurable retention period and dry-run mode for maintenance operations

Reviewed Changes

Copilot reviewed 23 out of 25 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
phone_verify/models.py	Added `is_expired` property to check if security codes have expired based on settings
phone_verify/backends/base.py	Added `get_security_code_expiration()` helper function with deprecation warning for old setting name
phone_verify/services.py	Updated settings validation to accept either old or new expiration setting name; moved constants to separate module
phone_verify/constants.py	New file consolidating default values including `DEFAULT_SECURITY_CODE_EXPIRATION_SECONDS`
phone_verify/admin.py	Enhanced admin interface with `is_valid` computed field showing expiration status with boolean icons
phone_verify/management/commands/cleanup_phone_verifications.py	New management command for cleaning up old verification records with configurable retention
tests/test_models.py	Comprehensive tests for `is_expired` property with various scenarios including deprecated settings
tests/test_services.py	Tests for settings validation and deprecation warnings
tests/test_management_commands.py	Tests for cleanup command functionality including dry-run mode
tests/test_api.py	Added test for expiry check with deprecated setting
tests/test_settings.py	Updated to use new setting name
makemigrations.py	Updated to use new setting name
docs/*.rst	Updated all documentation to reference new setting name with deprecation notes
README.rst	Updated configuration examples to use new setting name
CHANGELOG.rst	Documented changes including deprecation and new features
.github/SECURITY.md	Updated security recommendations to reference new setting name

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-20T18:07:57Z

phone_verify/models.py

+    @property
+    def is_expired(self):
+        """Check if the security code has expired.
+
+        Uses SECURITY_CODE_EXPIRATION_SECONDS (preferred) or
+        SECURITY_CODE_EXPIRATION_TIME (deprecated) setting.
+        """
+        phone_settings = settings.PHONE_VERIFICATION
+        # Check for new setting name first, then fall back to old name
+        expiration_time = phone_settings.get(
+            "SECURITY_CODE_EXPIRATION_SECONDS",
+            phone_settings.get("SECURITY_CODE_EXPIRATION_TIME", DEFAULT_SECURITY_CODE_EXPIRATION_SECONDS)
+        )
+        expiration_datetime = self.created_at + timedelta(seconds=expiration_time)
+        return timezone.now() > expiration_datetime


The is_expired property doesn't issue a deprecation warning when using the old SECURITY_CODE_EXPIRATION_TIME setting, while get_security_code_expiration() in backends/base.py does. This creates an inconsistency where developers using the model property directly won't see the deprecation warning.

Consider moving get_security_code_expiration() to a separate module (e.g., phone_verify/utils.py or within phone_verify/constants.py) to avoid circular imports, then use it in both the model and the backend. This ensures consistent behavior and deprecation warnings across the codebase.

Copilot · 2025-11-20T18:07:57Z

tests/test_models.py

+def test_sms_verification_is_expired_true(backend):
+    """Test that an old verification is expired."""
+    with override_settings(PHONE_VERIFICATION=backend):
+        backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 1
+
+        sms_verification = f.create_verification(
+            security_code=SECURITY_CODE,
+            phone_number=PHONE_NUMBER,
+            session_token=SESSION_TOKEN,
+        )
+
+        time.sleep(2)
+
+        assert sms_verification.is_expired is True


[nitpick] The backend dictionary is mutated after being passed to override_settings. While this technically works because Django stores a reference to the dictionary, it's better practice to modify the dictionary before passing it to override_settings for clarity and consistency with other tests in the codebase.

Consider this pattern instead:

backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 1 with override_settings(PHONE_VERIFICATION=backend): sms_verification = f.create_verification(...)

Copilot · 2025-11-20T18:07:57Z

tests/test_models.py

+def test_sms_verification_is_expired_custom_expiration_time(backend):
+    """Test is_expired with custom expiration time."""
+    with override_settings(PHONE_VERIFICATION=backend):
+        backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 300
+
+        sms_verification = f.create_verification(
+            security_code=SECURITY_CODE,
+            phone_number=PHONE_NUMBER,
+            session_token=SESSION_TOKEN,
+        )
+
+        fake_expired_time = timezone.now() + timedelta(seconds=301)
+        with patch("django.utils.timezone.now", return_value=fake_expired_time):
+            assert sms_verification.is_expired is True
+
+        fake_valid_time = timezone.now() + timedelta(seconds=200)
+        with patch("django.utils.timezone.now", return_value=fake_valid_time):
+            assert sms_verification.is_expired is False
+


[nitpick] The backend dictionary is mutated after being passed to override_settings. While this technically works, it's better practice to modify the dictionary before passing it to override_settings for clarity and consistency with other tests in the codebase.

Consider:

backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 300 with override_settings(PHONE_VERIFICATION=backend): sms_verification = f.create_verification(...)

Copilot · 2025-11-20T18:07:58Z

tests/test_management_commands.py

+def test_cleanup_phone_verifications_no_old_records(backend):
+    """Test cleanup command when no old records exist."""
+    with override_settings(PHONE_VERIFICATION=backend):
+        backend["RECORD_RETENTION_DAYS"] = 30
+
+        f.create_verification(
+            security_code=SECURITY_CODE,
+            phone_number=PHONE_NUMBER,
+            session_token=SESSION_TOKEN,
+        )
+
+        out = StringIO()
+        call_command("cleanup_phone_verifications", stdout=out)
+
+        assert "No verification records older than 30 days found" in out.getvalue()
+        assert SMSVerification.objects.count() == 1


[nitpick] The backend dictionary is mutated after being passed to override_settings. For better clarity and consistency with other tests, modify the dictionary before the override_settings call:

backend["RECORD_RETENTION_DAYS"] = 30 with override_settings(PHONE_VERIFICATION=backend): f.create_verification(...)

Copilot · 2025-11-20T18:07:58Z

tests/test_management_commands.py

+def test_cleanup_phone_verifications_deletes_old_records(backend):
+    """Test cleanup command deletes old records."""
+    with override_settings(PHONE_VERIFICATION=backend):
+        backend["RECORD_RETENTION_DAYS"] = 30
+
+        recent_verification = f.create_verification(
+            security_code=SECURITY_CODE,
+            phone_number=PHONE_NUMBER,
+            session_token=SESSION_TOKEN,
+        )
+
+        old_verification = f.create_verification(
+            security_code="654321",
+            phone_number="+13478379633",
+            session_token="old-session-token",
+        )
+
+        old_date = timezone.now() - timedelta(days=31)
+        SMSVerification.objects.filter(id=old_verification.id).update(created_at=old_date)
+
+        assert SMSVerification.objects.count() == 2
+
+        out = StringIO()
+        call_command("cleanup_phone_verifications", stdout=out)
+
+        assert "Successfully deleted 1 verification record(s)" in out.getvalue()
+        assert SMSVerification.objects.count() == 1
+        assert SMSVerification.objects.filter(id=recent_verification.id).exists()
+        assert not SMSVerification.objects.filter(id=old_verification.id).exists()


[nitpick] The backend dictionary is mutated after being passed to override_settings. For better clarity and consistency, modify the dictionary before the override_settings call:

backend["RECORD_RETENTION_DAYS"] = 30 with override_settings(PHONE_VERIFICATION=backend): recent_verification = f.create_verification(...)

Copilot · 2025-11-20T18:07:58Z

tests/test_management_commands.py

+def test_cleanup_phone_verifications_dry_run(backend):
+    """Test cleanup command with dry-run flag."""
+    with override_settings(PHONE_VERIFICATION=backend):
+        backend["RECORD_RETENTION_DAYS"] = 30
+
+        verification = f.create_verification(
+            security_code=SECURITY_CODE,
+            phone_number=PHONE_NUMBER,
+            session_token=SESSION_TOKEN,
+        )
+
+        old_date = timezone.now() - timedelta(days=31)
+        SMSVerification.objects.filter(id=verification.id).update(created_at=old_date)
+
+        out = StringIO()
+        call_command("cleanup_phone_verifications", dry_run=True, stdout=out)
+
+        output = out.getvalue()
+        assert "DRY RUN" in output
+        assert "Would delete 1 verification record(s)" in output
+        assert SMSVerification.objects.count() == 1


[nitpick] The backend dictionary is mutated after being passed to override_settings. For better clarity and consistency, modify the dictionary before the override_settings call:

backend["RECORD_RETENTION_DAYS"] = 30 with override_settings(PHONE_VERIFICATION=backend): verification = f.create_verification(...)

tests/test_models.py

Copilot

Pull Request Overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-20T18:47:33Z

phone_verify/backends/base.py

-        if time_difference.seconds > django_settings.PHONE_VERIFICATION.get(
-            "SECURITY_CODE_EXPIRATION_TIME"
-        ):
+        if time_difference.seconds > get_security_code_expiration():


The expiration check using time_difference.seconds is incorrect. The .seconds attribute only returns the seconds component of the timedelta (0-86399), not the total seconds. For example, a 2-day timedelta would have .seconds = 0 (or a small value), not 172800 seconds, causing expired codes to be considered valid.

Use time_difference.total_seconds() instead to get the complete duration in seconds.

Suggested change

if time_difference.seconds > get_security_code_expiration():

if time_difference.total_seconds() > get_security_code_expiration():

Copilot · 2025-11-20T18:47:33Z

tests/test_services.py


 # phone_verify Stuff
 import phone_verify.services
+from phone_verify.backends.base import get_security_code_expiration


The import location is incorrect. get_security_code_expiration is defined in phone_verify.constants, not in phone_verify.backends.base. While this import will work due to Python's re-export behavior, it's misleading and can cause confusion.

Change the import to:

from phone_verify.constants import get_security_code_expiration

Suggested change

from phone_verify.backends.base import get_security_code_expiration

from phone_verify.constants import get_security_code_expiration

Copilot · 2025-11-20T18:47:34Z

tests/test_models.py

+    backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 1
+
+    with override_settings(PHONE_VERIFICATION=backend):


[nitpick] The test directly modifies the shared backend fixture dictionary without making a copy first. While this works because the fixture creates a deep copy for each parametrized backend, it's inconsistent with other tests in this file (e.g., lines 77-78, 95-98) that explicitly create a backend_copy before modification.

For consistency and clarity, create a copy before modification:

def test_sms_verification_is_expired_true(backend): """Test that an old verification is expired.""" backend_copy = backend.copy() backend_copy["SECURITY_CODE_EXPIRATION_SECONDS"] = 1 with override_settings(PHONE_VERIFICATION=backend_copy): ...

Suggested change

backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 1

with override_settings(PHONE_VERIFICATION=backend):

backend_copy = backend.copy()

backend_copy["SECURITY_CODE_EXPIRATION_SECONDS"] = 1

with override_settings(PHONE_VERIFICATION=backend_copy):

Copilot · 2025-11-20T18:47:34Z

tests/test_models.py

+
+def test_sms_verification_is_expired_custom_expiration_time(backend):
+    """Test is_expired with custom expiration time."""
+    backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 300


[nitpick] The test directly modifies the shared backend fixture dictionary without making a copy first. While this works because the fixture creates a deep copy for each parametrized backend, it's inconsistent with other tests in this file (e.g., lines 77-78, 95-98) that explicitly create a backend_copy before modification.

For consistency and clarity, create a copy before modification:

def test_sms_verification_is_expired_custom_expiration_time(backend): """Test is_expired with custom expiration time.""" backend_copy = backend.copy() backend_copy["SECURITY_CODE_EXPIRATION_SECONDS"] = 300 with override_settings(PHONE_VERIFICATION=backend_copy): ...

Copilot · 2025-11-20T18:47:34Z

tests/test_management_commands.py

+
+def test_cleanup_phone_verifications_no_old_records(backend):
+    """Test cleanup command when no old records exist."""
+    backend["RECORD_RETENTION_DAYS"] = 30


[nitpick] The test directly modifies the shared backend fixture dictionary without making a copy first. While this works because the fixture creates a deep copy for each parametrized backend, it's better practice to explicitly create a copy to avoid potential issues and improve code clarity.

For consistency, create a copy before modification:

def test_cleanup_phone_verifications_no_old_records(backend): """Test cleanup command when no old records exist.""" backend_copy = backend.copy() backend_copy["RECORD_RETENTION_DAYS"] = 30 with override_settings(PHONE_VERIFICATION=backend_copy): ...

Copilot · 2025-11-20T18:47:35Z

tests/test_management_commands.py

+    backend["RECORD_RETENTION_DAYS"] = 30
+
+    with override_settings(PHONE_VERIFICATION=backend):


[nitpick] The test directly modifies the shared backend fixture dictionary without making a copy first. While this works because the fixture creates a deep copy for each parametrized backend, it's better practice to explicitly create a copy to avoid potential issues and improve code clarity.

For consistency, create a copy before modification:

def test_cleanup_phone_verifications_deletes_old_records(backend): """Test cleanup command deletes old records.""" backend_copy = backend.copy() backend_copy["RECORD_RETENTION_DAYS"] = 30 with override_settings(PHONE_VERIFICATION=backend_copy): ...

Suggested change

backend["RECORD_RETENTION_DAYS"] = 30

with override_settings(PHONE_VERIFICATION=backend):

backend_copy = backend.copy()

backend_copy["RECORD_RETENTION_DAYS"] = 30

with override_settings(PHONE_VERIFICATION=backend_copy):

Copilot · 2025-11-20T18:47:35Z

tests/test_management_commands.py

+    backend["RECORD_RETENTION_DAYS"] = 30
+
+    with override_settings(PHONE_VERIFICATION=backend):


[nitpick] The test directly modifies the shared backend fixture dictionary without making a copy first. While this works because the fixture creates a deep copy for each parametrized backend, it's better practice to explicitly create a copy to avoid potential issues and improve code clarity.

For consistency, create a copy before modification:

def test_cleanup_phone_verifications_dry_run(backend): """Test cleanup command with dry-run flag.""" backend_copy = backend.copy() backend_copy["RECORD_RETENTION_DAYS"] = 30 with override_settings(PHONE_VERIFICATION=backend_copy): ...

Suggested change

backend["RECORD_RETENTION_DAYS"] = 30

with override_settings(PHONE_VERIFICATION=backend):

backend_copy = backend.copy()

backend_copy["RECORD_RETENTION_DAYS"] = 30

with override_settings(PHONE_VERIFICATION=backend_copy):

Copilot

Pull Request Overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-20T18:57:30Z

tests/test_models.py

+        time.sleep(2)
+
+        assert sms_verification.is_expired is True
+
+


Using time.sleep() in tests makes them slower and potentially flaky. The PR already uses freezegun in other tests (line 68). Consider using freeze_time here as well to make the test faster and more reliable. For example: with freeze_time(timezone.now() + timedelta(seconds=2)):

Suggested change

time.sleep(2)

assert sms_verification.is_expired is True

with freeze_time(timezone.now() + timedelta(seconds=2)):

assert sms_verification.is_expired is True

Copilot · 2025-11-20T18:57:30Z

tests/test_models.py

+        time.sleep(2)
+
+        # Should use the new setting (1 second), so should be expired
+        assert sms_verification.is_expired is True


Using time.sleep() in tests makes them slower and potentially flaky. The PR already uses freezegun in other tests (line 68). Consider using freeze_time here as well to make the test faster and more reliable. For example: with freeze_time(timezone.now() + timedelta(seconds=2)):

Suggested change

time.sleep(2)

# Should use the new setting (1 second), so should be expired

assert sms_verification.is_expired is True

with freeze_time(timezone.now() + timedelta(seconds=2)):

# Should use the new setting (1 second), so should be expired

assert sms_verification.is_expired is True

Copilot · 2025-11-20T18:57:31Z

tests/test_api.py

+            phone_number=PHONE_NUMBER,
+            session_token=SESSION_TOKEN,
+        )
+        time.sleep(2)


Using time.sleep() in tests makes them slower and potentially flaky. Consider using freezegun.freeze_time() (which is already added to dependencies in this PR) to make the test faster and more reliable. For example: with freeze_time(timezone.now() + timedelta(seconds=2)):

Copilot · 2025-11-20T18:57:31Z

phone_verify/management/commands/cleanup_phone_verifications.py

+                )
+            )
+            self.stdout.write("Records that would be deleted:")
+            for record in old_verifications[:10]:


[nitpick] The hardcoded limit of 10 records shown in dry-run mode could be made configurable or at least defined as a constant at the module level (e.g., DRY_RUN_PREVIEW_LIMIT = 10) to make it easier to adjust and more maintainable.

Copilot

Pull Request Overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CuriousLearner added 2 commits November 20, 2025 22:34

feat: Add is_expired property for verifications

3f03186

refactor to constants

b388f8d

CuriousLearner requested a review from Copilot November 20, 2025 18:00

Copilot started reviewing on behalf of CuriousLearner November 20, 2025 18:00 View session

Copilot finished reviewing on behalf of CuriousLearner November 20, 2025 18:06

Copilot AI reviewed Nov 20, 2025

View reviewed changes

refactor patch to use freeze time and other test fixes

d974748

CuriousLearner requested a review from Copilot November 20, 2025 18:41

Copilot started reviewing on behalf of CuriousLearner November 20, 2025 18:42 View session

Copilot finished reviewing on behalf of CuriousLearner November 20, 2025 18:46

Copilot AI reviewed Nov 20, 2025

View reviewed changes

Address review comments

8b946fe

CuriousLearner requested a review from Copilot November 20, 2025 18:54

Copilot started reviewing on behalf of CuriousLearner November 20, 2025 18:55 View session

Copilot finished reviewing on behalf of CuriousLearner November 20, 2025 18:56

Copilot AI reviewed Nov 20, 2025

View reviewed changes

more refactoring

6e5c1f6

CuriousLearner requested a review from Copilot November 20, 2025 19:10

Copilot started reviewing on behalf of CuriousLearner November 20, 2025 19:10 View session

Minor remaining fix

8bb48b7

Copilot finished reviewing on behalf of CuriousLearner November 20, 2025 19:13

CuriousLearner requested review from Copilot and removed request for Copilot November 20, 2025 19:14

Copilot AI reviewed Nov 20, 2025

View reviewed changes

Copilot started reviewing on behalf of CuriousLearner November 20, 2025 19:14 View session

Copilot finished reviewing on behalf of CuriousLearner November 20, 2025 19:16

CuriousLearner added 2 commits November 23, 2025 02:05

Minor refactoring for better readability

8065214

Clarify tests

4ec37a5

CuriousLearner requested a review from Copilot November 22, 2025 20:39

Copilot started reviewing on behalf of CuriousLearner November 22, 2025 20:40 View session

CuriousLearner marked this pull request as ready for review November 22, 2025 20:40

Copilot finished reviewing on behalf of CuriousLearner November 22, 2025 20:43

Copilot AI reviewed Nov 22, 2025

View reviewed changes

CuriousLearner merged commit 283a584 into master Nov 22, 2025
21 checks passed

CuriousLearner deleted the 54-expired-status-admin branch November 22, 2025 20:46

	if time_difference.seconds > get_security_code_expiration():
	if time_difference.total_seconds() > get_security_code_expiration():

	from phone_verify.backends.base import get_security_code_expiration
	from phone_verify.constants import get_security_code_expiration

		backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 1

		with override_settings(PHONE_VERIFICATION=backend):

-    backend["SECURITY_CODE_EXPIRATION_SECONDS"] = 1
-    with override_settings(PHONE_VERIFICATION=backend):
+    backend_copy = backend.copy()
+    backend_copy["SECURITY_CODE_EXPIRATION_SECONDS"] = 1
+    with override_settings(PHONE_VERIFICATION=backend_copy):

		backend["RECORD_RETENTION_DAYS"] = 30

		with override_settings(PHONE_VERIFICATION=backend):

feat: Add is_expired property for verifications #121

feat: Add is_expired property for verifications #121

Uh oh!

Conversation

CuriousLearner commented Nov 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!