Skip to content

Conversation

omarxp
Copy link
Contributor

@omarxp omarxp commented Nov 9, 2021

  • Add API Subscription
  • Add Gopay Tokenization
  • Add sample
  • Add test

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Some changes are needed before this PR can be approved, please update ya. Thanks

README.md Outdated
Comment on lines 356 to 357
}
create_subscription = subscription.create(param)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explain this on the readme: Before merchant can call API create subscription, merchant should first obtain the 1 click token (which you can link it to this docs: https://docs.midtrans.com/en/core-api/advanced-features?id=recurring-transaction-with-subscriptions-api )

@omarxp
Copy link
Contributor Author

omarxp commented Nov 9, 2021

@rizdaprasetya new commit
Screen Shot 2021-11-09 at 17 31 08

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Still need some updates, other than that looks good 👍

@omarxp
Copy link
Contributor Author

omarxp commented Nov 10, 2021

@rizdaprasetya updated with new commit

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Few things need to be fixed. Also the commit message is not very descriptive, as always reminded before, please use proper & descriptive commit message next time.

@omarxp
Copy link
Contributor Author

omarxp commented Nov 10, 2021

@rizdaprasetya I use https://dillinger.io as markdown editor, should be fix now
Screen Shot 2021-11-10 at 14 51 07

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Few changes needed @omarxp . thanks

README.md Outdated
Comment on lines 373 to 374
You will receive gopay payment token with get_payment_account API
Check Tokenization examples folder (/examples/tokenization)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Screen Shot 2021-11-10 at 16 52 51

The example link doesn't get rendered properly, there is no spacing or proper dot punctuation between 2 sentences. Please fix & be more careful.

Also proper grammar should be:

You will receive gopay payment token using get_payment_account API call.

@rizdaprasetya
Copy link
Collaborator

rizdaprasetya commented Nov 10, 2021

@rizdaprasetya I use https://dillinger.io as markdown editor, should be fix now Screen Shot 2021-11-10 at 14 51 07

The render result of that tool seems different than Github, always test from user perspective, user will view it directly from Github, so you should too. All the screenshot I took from Github's markdown render.

@omarxp
Copy link
Contributor Author

omarxp commented Nov 11, 2021

@rizdaprasetya updated with new commit

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Still found some issue, please fix @omarxp

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

additional feedback

@omarxp
Copy link
Contributor Author

omarxp commented Nov 11, 2021

@rizdaprasetya please help to check again

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

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

Great, looks good now. Please merge and release, thanks! @omarxp (will share account creds via Slack)

@omarxp omarxp merged commit 7f8b615 into Midtrans:master Nov 11, 2021
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.

2 participants