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

Add plan and subscription resources #60

Merged
merged 4 commits into from
Sep 23, 2022
Merged

Conversation

gustav0
Copy link
Contributor

@gustav0 gustav0 commented Sep 13, 2022

Hey, before asking for an actual merge, I would like to know about 2 things:

  1. Is there any way to delete Plan and Subscription objects? Documentations doesn't show anything about it.
  2. Whenever I try to update and existing subscription, I get an 404 error with the following message:
    The template with id {plan_id} does not exist
    Do you have any idea why is this happening? I made sure the plan actually exists.

Besides that, I believed I could implement a lot more tests, but I don't think they will be useful as our goal is to verify the API calls are being made correctly.

Anything you guys think needs some change, just let me know, and I will get to it.

@lucmantovani
Copy link

Hello, @gustav0!

Thank you for your contribution!
We will review your pull request as soon as possible.

About your questions:

  1. It is not possible through the API. The only way to manage your plans and subscriptions is through the website
  2. It is hard to say for sure... It could be some temporary instability or some other issue. Can you give more details about the test? Did you use test, production credentials or tried with both?

@gustav0
Copy link
Contributor Author

gustav0 commented Sep 15, 2022

Hey @lucmantovani, thanks for your response!

Alright, I was just checking about the delete because you guys have deletion methods in this sdk that the API doesn't show.

About question number 2, I used the very same API ACCESS TOKEN you guys used in all your unit tests:
APP_USR-558881221729581-091712-44fdc612e60e3e638775d8b4003edd51-471763966

The test was about changing the reason attribute for an existing subscription. You can find test in the test_subscription.py file included in the pull request, at the begining of said test theres a comment with a #TODO, and the test is commented.

mercadopago/resources/plan.py Outdated Show resolved Hide resolved
mercadopago/resources/plan.py Outdated Show resolved Hide resolved
mercadopago/resources/plan.py Outdated Show resolved Hide resolved
mercadopago/resources/plan.py Outdated Show resolved Hide resolved
mercadopago/resources/subscription.py Outdated Show resolved Hide resolved
mercadopago/resources/subscription.py Outdated Show resolved Hide resolved
mercadopago/resources/subscription.py Outdated Show resolved Hide resolved
mercadopago/resources/subscription.py Outdated Show resolved Hide resolved
mercadopago/resources/subscription.py Outdated Show resolved Hide resolved
tests/test_subscription.py Outdated Show resolved Hide resolved
@eltinMeli
Copy link
Contributor

@gustav0 thanks for contributing our Python SDK, I left some comments about Lint, link to the docs and a tweak to the path that updates the subscription.

Any doubt, I am available.

Saludos

@gustav0 gustav0 marked this pull request as ready for review September 20, 2022 23:41
@gustav0
Copy link
Contributor Author

gustav0 commented Sep 21, 2022

@eltinMeli all set, linting is good and tests are 100% working.

@eltinMeli
Copy link
Contributor

@gustav0 the lint didn't go through a checker that was deprecated, you can remove it in your PR according to this other PR so we can proceed with the approval please.

@gustav0
Copy link
Contributor Author

gustav0 commented Sep 23, 2022

@eltinMeli done.

@eltinMeli eltinMeli merged commit 2e811c3 into mercadopago:master Sep 23, 2022
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