fix: remove and resolve `FIXME`s and `TODO`s from db module #174

NishantJoshi00 · 2022-12-19T12:43:23Z

Type of Change

Enhancement

Description

This change addresses todo!() in the MockDb interface implementation and group them under issue #172 . It fixes and removes other FIXMEs from the db folder.

Additional Changes

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

Motivation and Context

Following the initiative of removing TODOs and FIXMEs.
Linked issues:

fix(router_env): resolve/remove FIXME's and redundent files #168

How did you test it?

Checklist

I formatted the code cargo +nightly fmt
I addressed lints thrown by cargo clippy
I reviewed submitted code

SanchithHegde · 2022-12-19T14:08:04Z

crates/router/src/db/payment_attempt.rs

@@ -340,10 +346,9 @@ mod storage {
                        "{}_{}",
                        payment_attempt.payment_id, payment_attempt.merchant_id
                    );
-                    // TODO: need to add an application generated payment attempt id to distinguish between multiple attempts for the same payment id
-                    // Check for database presence as well Maybe use a read replica here ?


Are you sure this is not required either?

It's fixed, below.

I'm referring to this comment:

Check for database presence as well [...]

I'll check it out, I also feel it's an actionable

I have done the change, please take a look and suggest improvement, if any.

need to add an application generated payment attempt id to distinguish between multiple attempts for the same payment id

Now that I re-read this comment, I don't think we're determining multiple attempts to the same payment ID in our code, are we?

Looks like we're just setting the attempt_id to be an UUID here:
https://github.com/juspay/orca/blob/03d0cb1f5e256da6973932f388bab8d9e758bda6/crates/router/src/core/payments/operations/payment_create.rs#L437

This comment was regarding how it is being saved in the kv, previously it was only marked as pa, but later it was modified to be pa_{} in the redis hashmap

But, yes! Currently we only have one-to-one mapping between payment_intent and payment_attempt

This comment was regarding how it is being saved in the kv, previously it was only marked as pa, but later it was modified to be pa_{} in the redis hashmap

Ahh right, my bad, I misinterpreted it.

TODO: need to add an application generated payment attempt id to distinguish between multiple attempts for the same payment id

is this talking about adding attempt id to the key?

ashokkjag · 2022-12-20T08:24:35Z

can be remove the allow_panics too?

crates/router/src/db/queue.rs

ashokkjag · 2022-12-20T08:23:49Z

crates/router/src/db/payment_attempt.rs

@@ -340,10 +346,9 @@ mod storage {
                        "{}_{}",
                        payment_attempt.payment_id, payment_attempt.merchant_id
                    );
-                    // TODO: need to add an application generated payment attempt id to distinguish between multiple attempts for the same payment id
-                    // Check for database presence as well Maybe use a read replica here ?


TODO: need to add an application generated payment attempt id to distinguish between multiple attempts for the same payment id

is this talking about adding attempt id to the key?

NishantJoshi00 · 2022-12-20T09:01:54Z

Hey @ashokkjag, this comment is regarding the field value used to store the payment attempt in the hash map, earlier it was stored as "pa" but in case of multiple payment attempt this would fail. That was the intent of the comment, but it seems someone already solved it before this PR.

NishantJoshi00 added 3 commits December 19, 2022 17:46

fix: issue linkage for MockDb

0364029

fix: support multiple pa for pi while redis insert

4e4063c

fix: resolve clippy::unwrap warnings

344bceb

NishantJoshi00 added A-core Area: Core flows S-waiting-on-author Status: This PR is incomplete or needs to address review comments S-waiting-on-review Status: This PR has been implemented and needs to be reviewed C-refactor Category: Refactor labels Dec 19, 2022

NishantJoshi00 requested review from jarnura and SanchithHegde December 19, 2022 12:43

NishantJoshi00 self-assigned this Dec 19, 2022

NishantJoshi00 removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Dec 19, 2022

fix: remove remaining todos for list API

86a85f6

jarnura previously approved these changes Dec 19, 2022

View reviewed changes

jarnura added S-awaiting-merge S-needs-conflict-resolution Status: This PR needs conflicts to be resolved by the author and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed S-awaiting-merge labels Dec 19, 2022

SanchithHegde reviewed Dec 19, 2022

View reviewed changes

Merge branch 'main' of https://github.com/juspay/orca into db-fixmes

c6e76eb

NishantJoshi00 dismissed jarnura’s stale review via c6e76eb December 19, 2022 15:37

NishantJoshi00 added the S-in-progress Status: Implementation is underway label Dec 19, 2022

fix: use read replica for in KVonly pa action

03d0cb1

NishantJoshi00 added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed and removed S-in-progress Status: Implementation is underway S-needs-conflict-resolution Status: This PR needs conflicts to be resolved by the author labels Dec 19, 2022

NishantJoshi00 requested a review from jarnura December 19, 2022 17:52

fix: remove read_replica lookup

6462551

ashokkjag reviewed Dec 20, 2022

View reviewed changes

NishantJoshi00 added 2 commits December 20, 2022 14:35

fix: remove from the code

24dab82

Merge branch 'main' into db-fixmes

3b08c6e

ashokkjag approved these changes Dec 20, 2022

View reviewed changes

jarnura approved these changes Dec 20, 2022

View reviewed changes

jarnura added S-awaiting-merge and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Dec 20, 2022

jarnura merged commit 76b47e6 into main Dec 20, 2022

jarnura deleted the db-fixmes branch December 20, 2022 12:25

jarnura removed the S-awaiting-merge label Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove and resolve `FIXME`s and `TODO`s from db module #174

fix: remove and resolve `FIXME`s and `TODO`s from db module #174

NishantJoshi00 commented Dec 19, 2022

SanchithHegde Dec 19, 2022

NishantJoshi00 Dec 19, 2022

SanchithHegde Dec 19, 2022

NishantJoshi00 Dec 19, 2022

NishantJoshi00 Dec 19, 2022 •

edited

Loading

SanchithHegde Dec 19, 2022

NishantJoshi00 Dec 19, 2022

NishantJoshi00 Dec 19, 2022

SanchithHegde Dec 19, 2022

ashokkjag Dec 20, 2022

ashokkjag commented Dec 20, 2022

ashokkjag Dec 20, 2022

NishantJoshi00 commented Dec 20, 2022

fix: remove and resolve FIXMEs and TODOs from db module #174

fix: remove and resolve FIXMEs and TODOs from db module #174

Conversation

NishantJoshi00 commented Dec 19, 2022

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NishantJoshi00 Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokkjag commented Dec 20, 2022

Choose a reason for hiding this comment

NishantJoshi00 commented Dec 20, 2022

fix: remove and resolve `FIXME`s and `TODO`s from db module #174

fix: remove and resolve `FIXME`s and `TODO`s from db module #174

NishantJoshi00 Dec 19, 2022 •

edited

Loading