v6 - Improve expiry date input/output transformations and validation#2654
v6 - Improve expiry date input/output transformations and validation#2654
Conversation
AdyenAutomationBot
added
the
Chore
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how expiry dates are handled within the card component. The changes aim to improve the robustness of input parsing, provide a clearer user experience through automatic formatting, and simplify the underlying validation logic. By introducing dedicated parsing and transformation utilities and removing an unnecessary data model, the code becomes more maintainable and less prone to parsing errors, ensuring a more reliable and user-friendly expiry date input process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
jreij
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of expiry dates within the card component. It introduces a new ExpiryDateParser to parse raw 4-digit expiry date strings (MMYY) into separate month and year components. The CardExpiryDateValidator is updated to validate expiry dates using these separate month and year strings, replacing the previous MM/yy string-based validation. The UI input for expiry dates is enhanced with new InputTransformation and OutputTransformation classes to automatically format the input with a '/' separator and ensure only digits are entered. The ExpiryDate data class and related extensions are removed, streamlining the data model. Unit tests have been added for the new ExpiryDateParser and updated for CardExpiryDateValidator and CardComponentStateValidator to reflect the changes in expiry date handling. A review comment suggests that the ExpiryDateParser function should include validation to ensure its input string contains only digits, aligning with its documentation and preventing unexpected behavior.
| if (expiryDate.length != EXPIRY_DATE_MAX_LENGTH_NO_SEPARATORS) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
The function's KDoc states that it parses "digit only input", but there's no validation to ensure the input string contains only digits. This could lead to unexpected behavior if non-digit characters are passed, as take(2) and takeLast(2) would just take any characters. To make the function more robust and align with its documentation, you should add a check to ensure the input consists only of digits.
| if (expiryDate.length != EXPIRY_DATE_MAX_LENGTH_NO_SEPARATORS) { | |
| return null | |
| } | |
| if (expiryDate.length != EXPIRY_DATE_MAX_LENGTH_NO_SEPARATORS || !expiryDate.all { it.isDigit() }) { | |
| return null | |
| } |
There was a problem hiding this comment.
Since this function is internal, we assume the string has already been validated before being passed here.
jreij
force-pushed
the
chore/expiry-date-input-output-transformations
branch
from
e6c0207 to
da5eef3
Compare
COSDK-921
jreij
force-pushed
the
chore/expiry-date-input-output-transformations
branch
from
da5eef3 to
517b843
Compare
✅ No public API changes |
|
| } | ||
| val expiryMonth = expiryDate.take(2) | ||
| val shortYear = expiryDate.takeLast(2) | ||
| val expiryYear = if (returnFullYear) "$YEAR_PREFIX$shortYear" else shortYear |
There was a problem hiding this comment.
This will work for a long time, but wouldn't it be better to make this dynamic? Afaik SimpleDateFormat automatically handles century rollovers. You could do something like:
val expiryYear = if (returnFullYear) {
try {
val date = SimpleDateFormat("yy", Locale.ROOT).parse(shortYear)
date?.let { SimpleDateFormat("yyyy", Locale.ROOT).format(it) } ?: return null
} catch (e: ParseException) {
return null
}
} else {
shortYear
}There was a problem hiding this comment.
Maybe we can even replace this whole piece with SimpleDateFormat("MM/yy", Locale.ROOT) and use that to retrieve the month and year
| // Calendar months are 0-based (January is 0), so we subtract 1 | ||
| set(Calendar.MONTH, month - 1) | ||
| // add 2000 to the 2-digit year | ||
| set(Calendar.YEAR, YEAR_2000 + year) |
There was a problem hiding this comment.
Same here. The ExpiryDate class has some logic to create the expiry year dynamically. Maybe we could use that for inspiration?
3 participants
Description
Checklist
Ticket Number
COSDK-921