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

TDL-19325: Add missing fields #32

Merged
merged 9 commits into from
Jul 20, 2022
Merged

Conversation

hpatel41
Copy link
Contributor

Description of change

TDL-19325: Add missing fields

  • Added missing fields and newly added fields due to API Upgrade.

Manual QA steps

Risks

Rollback steps

  • revert this branch

@hpatel41 hpatel41 marked this pull request as draft June 20, 2022 09:08
@hpatel41 hpatel41 marked this pull request as ready for review June 22, 2022 05:49
kwargs['headers']['X-Recharge-Version'] = '2021-11'
kwargs['headers']['X-Recharge-Version'] = '2021-11'
# Products and Shop are not supported in new version: 2021-11
if path in ['products', 'shop']:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove shop from here as it is replaced with store in other PR.

Copy link
Contributor Author

@hpatel41 hpatel41 Jun 29, 2022

Choose a reason for hiding this comment

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

We will face 400 error when we run shop on 2021-11, but it will be replaced in the PR #27

# we will get empty records so adding 'X-Recharge-Version' for 'collections' API call
if path == 'collections':
kwargs['headers']['X-Recharge-Version'] = '2021-11'
kwargs['headers']['X-Recharge-Version'] = '2021-11'

Choose a reason for hiding this comment

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

We also removed the above comment, is it intensional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, now all streams except products and shop will run on 2021-11.

@@ -99,7 +99,7 @@ def test_config_provided_request_timeout(self, mock_get, mock_request):
client = RechargeClient(**config)
client.request("GET", "dummy_path")

mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=100.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'User-Agent': 'dummy_ua'})
mock_request.assert_called_with('GET', 'https://api.rechargeapps.com/dummy_path', stream=True, timeout=100.0, headers={'X-Recharge-Access-Token': 'dummy_at', 'Accept': 'application/json', 'X-Recharge-Version': '2021-11', 'User-Agent': 'dummy_ua'})

Choose a reason for hiding this comment

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

can we generalise the headers for all the unit tests? it will reduce the line length and improve code readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RushiT0122 Updated the test case to code to generalize params.

Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Can we generalise the headers for all the unit tests? It will reduce the line length and improve code readability.

@hpatel41 hpatel41 requested a review from RushiT0122 July 13, 2022 12:48
Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Changes look good and approving the PR.

Just a suggestion, in unittests like request timeout, where all tests are similar expect the test data. In such cases can we single test implementation and passing timeout value and expectation as test data?

@hpatel41
Copy link
Contributor Author

Changes look good and approving the PR.

@RushiT0122 Updated the PR as per suggestion. Can you please re-review it.

@hpatel41 hpatel41 requested a review from RushiT0122 July 14, 2022 06:51
Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

In request timeout unittest, test data is declared as a local variable and it will stop execution once it encounters first failure.

Ideally test should run separately for each test data. You can parameterise the unittests to implement it.

@hpatel41
Copy link
Contributor Author

In request timeout unittest, test data is declared as a local variable and it will stop execution once it encounters first failure.

Ideally test should run separately for each test data. You can parameterise the unittests to implement it.

@RushiT0122 Updated the request timeout test case to use parameterized test case.

@hpatel41 hpatel41 requested a review from RushiT0122 July 14, 2022 13:02
Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Looks good now!

@hpatel41 hpatel41 merged commit 9c11d80 into crest-master Jul 20, 2022
@hpatel41 hpatel41 mentioned this pull request Jul 20, 2022
RushiT0122 pushed a commit that referenced this pull request Aug 4, 2022
* TDL-19329: Update Pagination to cursor based for certain streams (#34)

* updated the pagination to cursor-based from page-based

* resolved unittests failure

* added unittests

* updated code and unittests

* TDL-19325: Add missing fields (#32)

* added missing fields and newly added fields due to api upgrade

* resolve unittest failure

* added missing fields and updated datatype for some fields

* added some missing fields

* updated code to use old API verison for onetimes stream

* updated unittest as per comments

* updated the request timeout unittest

* updated the request timeout unittest to use parameterized test data

* TDL-19414: Update datatype for some fields (#33)

* updated datatype for some fields

* resolved unittest failure

* updated schema

* added code to use old verison for onetimes stream

* updated unittests

* TDL-19326: Add missing integration tests (#31)

* Added missing integration test and missing assertions

* Ignored some fields in all_field test

* Updated code formatting for start_date test

* Updated code comments

* Added tap-tester test for interrupted sync

* Updated comment

Co-authored-by: harshpatel4crest <harsh.patel4@crestdatasys.com>

* TDL-19330: Add error logging (#30)

* added custom error handling

* updated logger.error message

* resolve pylint error

* updated 429 error class name

* updated unittests

* updated unittests

* resolved comments

* updated test cases with parameterized tests

* updated config.yml file

* updated unittests

* updated unittests

* TDL-19413: Products stream will not be supported in new version for certain PIMs (#29)

* added code to use old API for products strean

* updated unittests

* updated products schema

* updated comment

* TDL-19328: Change Shop API (#27)

* updated shop endpoint to store

* updated shop endpoint to /store

* updated readme file and changed shop to store

* added a field in store stream

* added default_api_version field in shop schema

Co-authored-by: Harsh Patel4 <harsh.patel4@CDSYS.LOCAL>

* updated the all fields test

* reverted the datatype change for a field in discounts stream

Co-authored-by: savan-chovatiya <80703490+savan-chovatiya@users.noreply.github.com>
Co-authored-by: Harsh Patel4 <harsh.patel4@CDSYS.LOCAL>
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.

5 participants