Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in FinTS TIME field formatting where months (MM) were incorrectly used instead of minutes (mm), causing the time format to be non-compliant with the FinTS 3.0 specification.
- Changed time format string from "HHMMss" to "HHmmss" to use minutes instead of months
- Added comprehensive unit tests to verify correct time formatting behavior
- Updated test mocks to align with the corrected format
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/fints/src/format.ts | Fixed time format string from "HHMMss" to "HHmmss" and added explanatory comment referencing FinTS 3.0 specification |
| packages/fints/src/tests/test-pin-tan-client.ts | Updated mock implementations to use the corrected "HHmmss" format |
| packages/fints/src/tests/test-format.ts | Added new test file with test cases verifying hours, minutes, and seconds are formatted correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| beforeEach(() => { | ||
| jest.spyOn(Format, "date").mockImplementation((date) => (date ? format(date, "HHMMss") : "20180101")); | ||
| jest.spyOn(Format, "time").mockImplementation((time) => (time ? format(time, "HHMMss") : "120000")); | ||
| jest.spyOn(Format, "date").mockImplementation((date) => (date ? format(date, "HHmmss") : "20180101")); | ||
| jest.spyOn(Format, "time").mockImplementation((time) => (time ? format(time, "HHmmss") : "120000")); |
There was a problem hiding this comment.
Switching the Format.date/time spies to format(..., "HHmmss") means the generated request payloads now encode minutes instead of months, but the stored snapshot in packages/fints/src/__tests__/__snapshots__/test-pin-tan-client.ts.snap still expects month-based values such as ...+N+120100+121000++. Running the existing test-pin-tan-client snapshot test will therefore fail until the snapshot (or the spy formatting) is updated to match the new HHmmss output.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Testing
Codex Task