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

MSPF-539: recurrent data storage protocol #556

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

aenglisc
Copy link
Contributor

@aenglisc aenglisc commented Apr 6, 2020

No description provided.

@aenglisc aenglisc requested a review from ciiol April 8, 2020 09:48
proto/domain.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
@aenglisc aenglisc requested a review from ciiol April 13, 2020 13:42
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/domain.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
@@ -1763,6 +1783,33 @@ service RecurrentPaymentTools {
2: RecurrentPaymentToolNotFound rec_payment_tool_not_found
3: EventNotFound event_not_found
)

RecurrentPaymentTool UpdateResource (
1: RecurrentPaymentToolID id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Мне пока не очень понятно, как адаптер (ну или HG) сможет при запросе на обновление карточных данных узнать идентификатор созданного когда-то в прошлом для хранения токена RecurrentPaymentTool. Ведь, насколько я понимаю, запрос снаружи придет с каким-то своим идентификатором (вероятно тот выдадут во время создания токена, не помню протокол). Как, на твой взгляд, должно происходить обновление?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Сейчас поперечитывал и пораздумывал — складывается ощущение, что возможно стоило бы отойти от идентификатора, и в качестве аргумента использовать непосредственно карточные данные. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Карточные данные не уникальны, их не получится использовать как идентификатор.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ну да, мысль была про то, чтобы обновлять все средства, где карточные данные совпадают с переданными.

Copy link
Contributor

@ciiol ciiol Apr 17, 2020

Choose a reason for hiding this comment

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

Как минимум мы не можем обновлять данные везде потому что держатель карты явно дает согласие на обновление данных в конкретной подписке. Это даже не говоря о том, что запрос снаружи придет с идентификатором subscriptionID

Copy link
Contributor

Choose a reason for hiding this comment

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

А зачем системе нужен этот идентификатор подписки? Он даже не уникален сам по себе.

Copy link
Contributor

Choose a reason for hiding this comment

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

Или задам вопрос иначе. Чем этот идентификатор для нас отличается от рекурентного токена и зачем нам их разделять?

Copy link
Contributor

Choose a reason for hiding this comment

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

Хотя да, наверное он нужен, чтобы потом можно было по нему снаружи сослаться на созданный токен и сохраненные данные карты.

Copy link
Contributor

Choose a reason for hiding this comment

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

Чтобы этот самый id подписки получить.

Но зачем он нужен там? Вроде пока никто не собирался обновлять данные старых подписок.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Если только для новых вводить — в принципе не нужен, да.

proto/proxy_provider.thrift Outdated Show resolved Hide resolved
proto/payment_processing.thrift Outdated Show resolved Hide resolved
proto/proxy_provider.thrift Outdated Show resolved Hide resolved
@aenglisc aenglisc requested a review from ciiol April 30, 2020 06:52
@kpy3 kpy3 force-pushed the MSPF-539/ft/recurrent_data_storage_protocol branch from 7be8dec to d50e883 Compare July 30, 2020 09:30
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
Comment on lines 66 to 67
1: CardNotTokenizable card_not_tokenizable
2: domain.Failure failure
Copy link
Contributor

Choose a reason for hiding this comment

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

А почему было решено отдельно выделить именно эту ошибку?

Copy link
Contributor

Choose a reason for hiding this comment

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

не все ошибки формализованы к сожалению, оставил в общем виде пока

Copy link
Contributor

Choose a reason for hiding this comment

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

Я скорее про card_not_tokenizable. Мне пока кажется странным, что это отдельная ошибка / отдельный класс ошибок.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ну наверное ты прав, уберу

proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
/**
* Токенизация банковской карты.
**/
PaymentSystemProxyResult Tokenize(1: InvoicePayment invoice_payment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Странно, а почему передается платеж (и только платеж)? Там как минимум еще нужны опции адаптера, его стейт и т.д.

Copy link
Contributor

Choose a reason for hiding this comment

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

В платеже, кажется есть всё что нужно для токенизации, айдишники будут в параметрах терминала, кажется тут нужно добавить опции адаптера и стейт

Copy link
Contributor

Choose a reason for hiding this comment

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

Ну обычно мы делаем другую структуру, куда, быть может, как одно из полей входит платеж. Просто периодически возникает желание добавить еще информации для адаптера.

Copy link
Contributor

Choose a reason for hiding this comment

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

К примеру, в платежный адаптер передается еще и инвойс

Copy link
Contributor

Choose a reason for hiding this comment

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

Ок, добавил структуру

proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
@@ -658,6 +663,35 @@ union ShopLocation {
1: string url
}

/** Данные, необходимые для идентификации и аутентификации в МПС API **/

union PaymentTokenProviderInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Этот тип нигде не используется. И должен ли? Вроде хотели хранить эти данные (и их часть) в параметрах терминала.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ну я пытался их формализовать как раз для параметров терминала или считаешь не нужно?

Copy link
Contributor

Choose a reason for hiding this comment

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

Убрал в итоге

Copy link
Contributor

Choose a reason for hiding this comment

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

Не убрал, вроде.

Copy link
Contributor

Choose a reason for hiding this comment

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

Точно убрал :)

@kpy3 kpy3 requested a review from ciiol September 18, 2020 07:50
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
proto/payment_system_provider.thrift Outdated Show resolved Hide resolved
proto/domain.thrift Show resolved Hide resolved
proto/payment_processing.thrift Show resolved Hide resolved

struct TokenizationSuccess {
1: PaymentTokenID token_id
2: PaymentTokenEnrollment token_enrollment
Copy link
Contributor

Choose a reason for hiding this comment

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

А что такое PaymentTokenEnrollment и зачем он нужен? Нигде больше он сейчас не используется.

Copy link
Contributor

Choose a reason for hiding this comment

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

оно используется в некоторых служебных вызовах например при манипуляциях с токеном, непонятно, правда нужен ли он будет нам или нет, решил что пусть пока сохраняется

/**
* Необходимые для авторизации данные
**/
typedef base.Opaque TokenCredentials
Copy link
Contributor

Choose a reason for hiding this comment

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

Странно, что это непрозрачные данные. Как адаптеры будут ими пользоваться?

Copy link
Contributor

Choose a reason for hiding this comment

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

у них разная структура и наполнение, пусть адаптеры сами решают как им хранить это

proto/payment_system_provider.thrift Outdated Show resolved Hide resolved

union FinishIntentSuccess {
1: TokenizationSuccess tokenization_success
2: GetTokenCredentialsSuccess credential_success
Copy link
Contributor

Choose a reason for hiding this comment

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

Учитывая, что GetTokenCredentials будет вызывать, вроде бы, платежный адаптер, мне представляется весьма нетривиальной задачей пробросить коллбек сквозь платежный адаптер. И, кажется, сейчас протокол в принципе не позволяет этого. Поставить коллбек при GetTokenCredentials можно, а обработать его не получится.

Copy link
Contributor

Choose a reason for hiding this comment

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

Это я к тому, что быть может, если получение данных синхронное, пока не добавлять это в протокол.

Copy link
Contributor

Choose a reason for hiding this comment

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

Поставить коллбек при GetTokenCredentials можно, а обработать его не получится.

Хотя, я тут подумал, обработать наверное получится.

/**
* Получить данные (ключи, криптограммы) для авторизации платежа
**/
PaymentSystemProviderResult GetTokenCredentials(1: PaymentTokenID token_id, 2: Context context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Если этот вызов будет делать платежный адаптер, то ему придется собирать Context из payment_proxy.PaymentInfo. Это может быть весьма непросто. Или даже невозможно – вроде мы не пытались делать так, чтобы в payment_proxy.PaymentInfo была вся информация из платежа.

Copy link
Contributor

Choose a reason for hiding this comment

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

Предлагаешь выпилить тут Context?

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.

3 participants