feat(cards): validate card security code and expiration #874

divinenaman · 2023-04-13T11:14:23Z

Type of Change

Description

Add basic validations for card security codes (CVC/CVV/CID code) and card expiration month and year. This logic resides in a new crate called cards. Validations included (as of now):

Card security code must be 3 or 4 digits only
Expiration month is valid (1-12)
Expiration year is a 4 digit number only
Card has not expired (expiration month and year correspond to a date in the future)

Additional Changes

This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

This fixes #607

How did you test it?

run cargo test --cards

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed submitted code
I added unit tests for my changes where possible
I added a CHANGELOG entry if applicable

SanchithHegde

Could you please move the logic in the constructors to the TryFrom trait implementations for CardSecurityCode, CardExpirationMonth and CardExpirationYear?

So:

CardSecurityCode::new() would become CardSecurityCode::try_from(u16)
CardExpirationMonth::new() would become CardExpirationMonth::try_from(u8)
CardExpirationYear::new() would become CardExpirationYear::try_from(u16)

And regarding the Serialize and Deserialize implementations, would you want to take them up in this PR or a separate one? Let us know if you have any questions.

crates/cards/src/lib.rs

crates/cards/Cargo.toml

crates/cards/src/lib.rs

vspecky

Also look into adding serde::{Serialize, Deserialize} implementations with appropriate error reporting. Thanks for taking this up!

crates/cards/src/lib.rs

crates/cards/tests/basic.rs

divinenaman · 2023-04-15T08:49:53Z

Could you please move the logic in the constructors to the TryFrom trait implementations for CardSecurityCode, CardExpirationMonth and CardExpirationYear?

So:
1. `CardSecurityCode::new()` would become `CardSecurityCode::try_from(u16)`

2. `CardExpirationMonth::new()` would become `CardExpirationMonth::try_from(u8)`

3. `CardExpirationYear::new()` would become `CardExpirationYear::try_from(u16)`

@SanchithHegde should I remove the new implementation and just implement a TryFrom instance or keep both ?

SanchithHegde · 2023-04-15T10:32:20Z

@SanchithHegde should I remove the new implementation and just implement a TryFrom instance or keep both ?

Yes, remove the new() method and have only a TryFrom implementation. new() would be suitable when the constructor is not expected to fail (doesn't return a Result), which clearly isn't the case here.

crates/cards/src/lib.rs

crates/cards/tests/basic.rs

crates/cards/Cargo.toml

crates/cards/src/lib.rs

SanchithHegde

Other than that, looks good to me! Thanks again for the PR!

crates/cards/Cargo.toml

crates/cards/tests/basic.rs

SanchithHegde

Other than that, looks good to me!

crates/cards/tests/basic.rs

vspecky · 2023-04-25T04:59:23Z

crates/cards/tests/basic.rs

+    let valid_card_security_code = CardSecurityCode::try_from(1234).unwrap();
+
+    // will panic on unwrap
+    let invalid_card_security_code = CardSecurityCode::try_from(12);
+
+    assert_eq!(*valid_card_security_code.peek(), 1234);
+    assert!(invalid_card_security_code.is_err());
+
+    let serialized = serde_json::to_string(&valid_card_security_code).unwrap();
+    assert_eq!(serialized, "1234");
+
+    let derialized = serde_json::from_str::<CardSecurityCode>(&serialized).unwrap();


Not a blocker but prefer .expect("what was expected") over .unwrap() since it makes finding the cause of panic easier.

vspecky

LGTM

SanchithHegde · 2023-04-25T18:14:26Z

@divinenaman Please address CI checks.

feat: cards crate

c34801a

divinenaman requested review from a team, jarnura and ashokkjag as code owners April 13, 2023 11:14

SanchithHegde assigned divinenaman Apr 13, 2023

SanchithHegde added C-feature Category: Feature request or enhancement S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Apr 13, 2023

SanchithHegde reviewed Apr 13, 2023

View reviewed changes

SanchithHegde requested review from vspecky and NishantJoshi00 April 13, 2023 17:47

SanchithHegde changed the title ~~feat: cards crate~~ feat(cards): validate card security code and expiration Apr 13, 2023

SanchithHegde added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Apr 13, 2023

NishantJoshi00 reviewed Apr 13, 2023

View reviewed changes

crates/cards/src/lib.rs Outdated Show resolved Hide resolved

crates/cards/src/lib.rs Outdated Show resolved Hide resolved

vspecky requested changes Apr 14, 2023

View reviewed changes

crates/cards/src/lib.rs Outdated Show resolved Hide resolved

crates/cards/tests/basic.rs Outdated Show resolved Hide resolved

jarnura added this to the April'23 release milestone Apr 14, 2023

fix(cards): changes

8db27b8

divinenaman force-pushed the dev branch from 6894f1b to 8db27b8 Compare April 15, 2023 18:30

SanchithHegde reviewed Apr 17, 2023

View reviewed changes

crates/cards/src/lib.rs Outdated Show resolved Hide resolved

feat(cards): add serialize & deserialize

d2528bb

SanchithHegde reviewed Apr 20, 2023

View reviewed changes

crates/cards/Cargo.toml Outdated Show resolved Hide resolved

crates/cards/tests/basic.rs Outdated Show resolved Hide resolved

crates/cards/tests/basic.rs Outdated Show resolved Hide resolved

SanchithHegde reviewed Apr 20, 2023

View reviewed changes

crates/cards/tests/basic.rs Show resolved Hide resolved

fix(cards): add unit tests

bfacf42

SanchithHegde reviewed Apr 20, 2023

View reviewed changes

crates/cards/tests/basic.rs Outdated Show resolved Hide resolved

divinenaman added 2 commits April 21, 2023 02:18

fix(cards): tests

9d49831

fix(cards): dependencies

95eeef3

SanchithHegde reviewed Apr 21, 2023

View reviewed changes

crates/cards/tests/basic.rs Outdated Show resolved Hide resolved

fix(cards): tests

f2fffb6

SanchithHegde previously approved these changes Apr 21, 2023

View reviewed changes

SanchithHegde requested a review from vspecky April 24, 2023 09:31

SanchithHegde added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Apr 24, 2023

vspecky reviewed Apr 25, 2023

View reviewed changes

vspecky previously approved these changes Apr 25, 2023

View reviewed changes

Merge branch 'main' into dev

49f4305

fix(cards): tests allow expect

7f75e79

divinenaman dismissed stale reviews from vspecky and SanchithHegde via 7f75e79 April 25, 2023 18:40

divinenaman added 2 commits April 26, 2023 01:33

fix(cards): resolve clippy warnings

db3f67a

fix(cards): fix formatting

ba24e41

SanchithHegde approved these changes Apr 25, 2023

View reviewed changes

vspecky approved these changes Apr 26, 2023

View reviewed changes

SanchithHegde mentioned this pull request Apr 27, 2023

[REFACTOR] Integrate the card expiration month/year and security code newtypes in request/response types #1012

Open

jarnura approved these changes May 2, 2023

View reviewed changes

jarnura added this pull request to the merge queue May 2, 2023

jarnura added S-ready-for-merge and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels May 2, 2023

Merged via the queue into juspay:main with commit 0b7bc7b May 3, 2023

SanchithHegde removed the S-ready-for-merge label May 3, 2023

Narayanbhat166 mentioned this pull request May 3, 2023

doc: changelog for release v0.5.9 #1037

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cards): validate card security code and expiration #874

feat(cards): validate card security code and expiration #874

divinenaman commented Apr 13, 2023 •

edited

Loading

SanchithHegde left a comment

vspecky left a comment

divinenaman commented Apr 15, 2023

SanchithHegde commented Apr 15, 2023

SanchithHegde left a comment

SanchithHegde left a comment

vspecky Apr 25, 2023

vspecky left a comment

SanchithHegde commented Apr 25, 2023

feat(cards): validate card security code and expiration #874

feat(cards): validate card security code and expiration #874

Conversation

divinenaman commented Apr 13, 2023 • edited Loading

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

SanchithHegde left a comment

Choose a reason for hiding this comment

vspecky left a comment

Choose a reason for hiding this comment

divinenaman commented Apr 15, 2023

SanchithHegde commented Apr 15, 2023

SanchithHegde left a comment

Choose a reason for hiding this comment

SanchithHegde left a comment

Choose a reason for hiding this comment

vspecky Apr 25, 2023

Choose a reason for hiding this comment

vspecky left a comment

Choose a reason for hiding this comment

SanchithHegde commented Apr 25, 2023

divinenaman commented Apr 13, 2023 •

edited

Loading