Repository navigation
STF-1906: Add phone verification inputs - #475
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 2 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding phone verification inputs to the billing and shipping request objects. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the dates at night
And stamps each phone detail just right
Billing, shipping fields appear
The clock rejects a leap too queer
The bunny hops, the tests shine bright!
Comment @coderabbitai help to get the list of available commands.
2792b9a to
a28ab6c
Compare
oschwald
changed the title
Add the optional phone_verification_method, phone_was_verification_successful, and phone_verification_time inputs to both the /billing and /shipping request objects. They describe the most recent verification of the phone number. Both objects share the same validation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The RFC 3339 check used only a regex. It accepted values that the web service rejects, such as 2026-02-30T25:61:61Z or an offset of +24:00. The check now uses the server's regex with an uppercase T and Z only, then parses the date and time to reject impossible values. This applies to /event/time and phone_verification_time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a28ab6c to
2e99c0c
Compare
oschwald
changed the title
horgh
left a comment
horgh
left a comment
There was a problem hiding this comment.
Looks good, with Claude nits
| "phone_country_code": _telephone_country_code, | ||
| "phone_number": str, | ||
| "phone_verification_method": _phone_verification_method, | ||
| "phone_verification_time": _rfc3339_datetime, |
There was a problem hiding this comment.
phone_verification_time uses the shared _rfc3339_datetime validator, so the client does not check the range in the spec: from the Unix epoch to two days in the future.
validate_transaction({"billing": {"phone_verification_time": "1969-12-31T23:59:59Z"}}) passes, and so does "2099-01-01T00:00:00Z". The server (validatePhoneVerificationTime) rejects both with PHONE_VERIFICATION_TIME_OUT_OF_RANGE, ignores the field, and scores the transaction without the signal. The caller gets no error.
The lower bound does not depend on the clock, so the client can check it with no clock-skew risk. The upper bound depends on the client clock, so skipping it is defensible, but the decision should be the same in all the sibling client PRs. The client also does not check the one-year range for /event/time, so if the intent is "format only, no range", the changelog should say that (see the HISTORY.rst comment).
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
No client checks this range, the same as for /event/time. The web service ignores a time outside the range and returns a warning. cf3ab80 changes the HISTORY entry, so it no longer says that the check matches the web service.
Reply by Claude Opus 5.5 on Greg's behalf.
| inputs to the ``/billing`` and ``/shipping`` request objects. They describe | ||
| the most recent verification of the phone number. | ||
| * The RFC 3339 check on ``/event/time`` and ``phone_verification_time`` now | ||
| rejects the same values as the web service. It rejects impossible dates |
There was a problem hiding this comment.
"rejects the same values as the web service" is not correct. The client still accepts values that the server rejects:
phone_verification_timebefore the Unix epoch or more than two days in the future.event.timeoutside the server range (for example2015-05-08T16:07:56Z, which is inGOOD_TIMES).- Strings longer than 255 characters (see the comment on the fraction group).
Suggest wording such as "now rejects more invalid values, such as ...", or add the missing checks.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Reworded in cf3ab80. It now says that the check rejects more values that the web service rejects.
Reply by Claude Opus 5.5 on Greg's behalf.
|
|
||
| _rfc3339_datetime = All( | ||
| Match( | ||
| r"(?a)\A\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?" |
There was a problem hiding this comment.
The fraction group (\.\d+)? has no length limit. The server calls getString(v, 255, false) before it parses the time, so it rejects a value longer than 255 characters.
"2026-10-01T14:30:00." + "1" * 300 + "Z" passes validate_transaction for event.time and phone_verification_time. The server rejects it with "the string is too long" and ignores the field. This is an edge case, but it conflicts with the changelog claim. A bound such as (\.\d{1,9})? (nanosecond precision, which Go keeps) would close it.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
We will leave this. It is an edge case, and the HISTORY entry no longer claims a full match.
Reply by Claude Opus 5.5 on Greg's behalf.
| BAD_TIMES = ( | ||
| "2015-05-08T16:07:56", | ||
| "2015-05-08 16:07:56Z", | ||
| "2026-02-30T25:61:61Z", |
There was a problem hiding this comment.
No BAD_TIMES case tests only an impossible day of the month. 2026-02-30T25:61:61Z also has hour 25 and minute and second 61, and 2026-13-45T10:00:00Z has month 13. Either one fails on the other fields first.
If _datetime_exists checked only field ranges (day 1 to 31), February 30 would pass and all tests would still pass. Suggest adding cases such as 2026-02-30T10:00:00Z, 2025-02-29T00:00:00Z (not a leap year) and 2026-04-31T00:00:00Z, so the "February 30" behavior in the changelog has a test.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Added in 4f6a9b1.
Reply by Claude Opus 5.5 on Greg's behalf.
| def _datetime_exists(s: str) -> str: | ||
| # Reject impossible values such as Feb 30 or 24:00. The regex already | ||
| # checked the fraction and the offset, so parse only the part before them. | ||
| datetime.strptime(s[:19], "%Y-%m-%dT%H:%M:%S") # noqa: DTZ007 |
There was a problem hiding this comment.
Nit: when strptime raises ValueError, voluptuous replaces it with a generic message:
Invalid transaction data: not a valid value for dictionary value @ data['event']['time']
The caller does not learn that the date or time is impossible. A regex failure at least shows the pattern. Raising voluptuous.Invalid("invalid date or time") here would give a useful message.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Done in 12450c7. The error is now "invalid date or time".
Reply by Claude Opus 5.5 on Greg's behalf.
| def test_phone_was_verification_successful(self) -> None: | ||
| self.check_bool(self.type, "phone_was_verification_successful") | ||
|
|
||
| def test_phone_verification_time(self) -> None: |
There was a problem hiding this comment.
Nit: this test covers only string inputs. test_phone_verification_method tests the int 1, but no test shows that a value that is not a string is rejected here, for example 1 or datetime(2026, 10, 1, tzinfo=UTC).
A Python caller can easily pass a datetime. Match rejects it now, but if a later change puts _datetime_exists first in All(...), s[:19] raises TypeError instead of a validation error, and no test fails.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Added in 4f6a9b1.
Reply by Claude Opus 5.5 on Greg's behalf.
The client does not check the time ranges or the length limit of the web service, so it does not reject every value that the web service rejects. Say that it now rejects more of these values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add times that fail only on the calendar day. Also test that the int 1 and a datetime object are rejected for event.time and phone_verification_time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Before, the error was "not a valid value". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds three optional inputs to the
/billingand/shippingrequest objects:phone_verification_method(delivered_code,network, orother),phone_was_verification_successful(boolean), andphone_verification_time(RFC 3339 date-time). They describe the most recent verification of the phone number. Both objects share one validation schema, and each value is checked.The second commit makes the RFC 3339 check, which
/event/timealso uses, reject what the web service rejects: impossible dates and times such as2026-02-30T25:61:61Z, offsets of 24 hours or more, and a lowercasetorz.🤖 Generated with Claude Code
Summary by CodeRabbit