fix: correct UPS and USPS time parsing - #43
Merged
Conversation
UPS reports a 12-hour clock as "10:53 P.M.", but the format string read it as `HH:mm`, so the meridiem was ignored and every afternoon event landed twelve hours early. Seven of the twelve activities in the recorded fixture are P.M., so this was wrong far more often than not. dayjs only matches "PM", not "P.M.", so the periods have to be stripped before parsing with `h:mm A`. USPS gets the same treatment for whitespace: runs of spaces inside the date cell are collapsed before parsing, and the format list now also accepts a space after the second comma. Both were reported in #35 by @aldin-alagic in 2022. That PR targeted the old `lib/` tree and could no longer be merged after the v3 rewrite, so the fixes are reapplied here with a regression test for each — the existing sweep only checked that a timestamp parsed, not that it landed on the right hour.
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
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
Fixes two time-parsing bugs in the UPS and USPS couriers, both originally reported in #35.
Background
@aldin-alagic opened #35 in December 2022 with fixes for exactly these two bugs. It sat unreviewed, and by the time v3 landed the sources had moved from
lib/tosrc/, so the patch no longer applied. The bugs were still there — the v3 rewrite carried them over unchanged, and the comparison I ran against v2 could not catch them because both versions were wrong in the same way.Changes
10:53 P.M., but the format string read it asHH:mm. The meridiem was ignored, so every afternoon event was recorded twelve hours early. Seven of the twelve activities in the recorded fixture are P.M., so this was wrong more often than it was right. dayjs'sAtoken only matchesPM, notP.M., so the periods are stripped before parsing withh:mm A.Key files
src/courier/ups.ts,src/courier/usps.ts,test/checkpoint-time.test.tsRollout / Follow-up
Folded into the unreleased 3.0.0 rather than a patch release, since v3 has not been published yet.
Closes #35.
Risks
UPS timestamps that consumers stored from earlier versions will not match what this returns for the same shipment — the old values were simply wrong for afternoon events.
Verification
Reverting either fix makes the new tests fail, which is how the UPS bug was confirmed rather than assumed.