Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(customer): Customer kv impl #4267

Merged
merged 3 commits into from
Apr 12, 2024
Merged

feat(customer): Customer kv impl #4267

merged 3 commits into from
Apr 12, 2024

Conversation

akshay-97
Copy link
Contributor

@akshay-97 akshay-97 commented Apr 1, 2024

Type of Change

  • Bugfix
  • New feature
  • [ s] Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Add kv support for customer table, so that writes and updates dont go to master db

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

Since payment flows can write to customer table, this entity needs to be supported in KV

How did you test it?

enable kv config for test merchant

  1. create customer
    curl --location 'http://localhost:8080/customers' \ --header 'Content-Type: application/json' \ --header 'Accept: application/json' \ --header 'api-key:***' \ --data-raw '{ "email": "guest@example.com", "name": "John Doe", "phone": "999999999", "phone_country_code": "+65", "description": "First customer", "metadata": { "udf1": "value1", "new_customer": "true", } }'

  2. create payment_method with same customer
    curl --location 'http://localhost:8080/payment_methods' \ --header 'Content-Type: application/json' \ --header 'Accept: application/json' \ --header 'api-key:***' \ --data '{ "payment_method": "card", "payment_method_type": "credit", "payment_method_issuer": "Visa", "card": { "card_number": "4242424242424242", "card_exp_month": "10", "card_exp_year": "25", "card_holder_name": "John Doe" }, "customer_id": <cust_id>, "metadata": { "city": "NY", "unit": "245" } }'

check drainer for errors

check in db after drainer has run for existence of payment method and customer info

Checklist

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

@akshay-97 akshay-97 requested review from a team as code owners April 1, 2024 15:03
Comment on lines 18 to 43
pub async fn update(
self,
conn: &PgPooledConn,
customer: CustomerUpdateInternal,
) -> StorageResult<Self> {
match generics::generic_update_by_id::<<Self as HasTable>::Table, _, _, _>(
conn,
(self.customer_id.clone(), self.merchant_id.clone()),
customer,
)
.await
{
Err(error) => match error.current_context() {
errors::DatabaseError::NoFieldsToUpdate => {
generics::generic_find_by_id::<<Self as HasTable>::Table, _, _>(
conn,
(self.customer_id, self.merchant_id),
)
.await
}
_ => Err(error),
},
result => result,
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This look like a redundant function

@@ -186,10 +187,11 @@ pub async fn delete_customer(
) -> errors::CustomerResponse<customers::CustomerDeleteResponse> {
let db = &state.store;

db.find_customer_by_customer_id_merchant_id(
let _customer = db.find_customer_by_customer_id_merchant_id(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the underscore here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Comment on lines 87 to 89
let merchant_account = db.find_merchant_account_by_merchant_id(merchant_id, key_store)
.await
.to_not_found_response(errors::ApiErrorResponse::MerchantAccountNotFound)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass the merchant account from the previous function. No need of another DB call here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@@ -1157,11 +1157,13 @@ pub async fn get_customer_from_details<F: Clone>(
match customer_id {
None => Ok(None),
Some(c_id) => {
let merchant_account = db.find_merchant_account_by_merchant_id(merchant_id , merchant_key_store).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass it from prev function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@@ -1309,13 +1311,15 @@ pub async fn create_customer_if_not_exist<'a, F: Clone, R, Ctx>(
.customer_id
.or(payment_data.payment_intent.customer_id.clone());

let merchant_account = db.find_merchant_account_by_merchant_id(merchant_id, key_store).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

let updated_object = match storage_scheme {
MerchantStorageScheme::PostgresOnly => database_call().await,
MerchantStorageScheme::RedisKv => {
let key = format!("mid_{}_cust_{}", merchant_id, customer_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the key be merchant_id_customer_id? I dont think this serves the purpose of sharding all the data with single key. Should we think of something else

cc: @jarnura

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was thinking mandate data can also come under mid_cust_id

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for address it's mid_pid because only payment related address is going to KV

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does that work, would address be changed or created only in payments flow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the addresses which are going through KV flow are only inserted during payments

@akshay-97 akshay-97 changed the title [enhancement] Customer kv impl [feat]: Customer kv impl Apr 12, 2024
@akshay-97 akshay-97 changed the title [feat]: Customer kv impl feat(customer): Customer kv impl Apr 12, 2024
@Gnanasundari24 Gnanasundari24 added this pull request to the merge queue Apr 12, 2024
@akshay-97 akshay-97 self-assigned this Apr 12, 2024
@akshay-97 akshay-97 added this to the March 2024 milestone Apr 12, 2024
Merged via the queue into main with commit c980f01 Apr 12, 2024
16 of 21 checks passed
@Gnanasundari24 Gnanasundari24 deleted the customer_kv_impl branch April 12, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants