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

[WISHLISTS-25] fix the wishlist api #95

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

auroraaaashen
Copy link
Contributor

What problem is this solving?

Bugfix for the wishlist API, the wishlist API is using the scroll api . There were no subsequent scroll requests so the API couldn't return all the wishlist records. This change sets the scroll size to 200 so that the scroll will return 200 records in each call.

How to test it?

Add a product to your wishlist so that your record will be the most recent one. Then download the wishlist sheet and search to see if your newest record is there.

@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Feb 25, 2022

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@vtex-io-docs-bot
Copy link

vtex-io-docs-bot bot commented Feb 25, 2022

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

@auroraaaashen auroraaaashen force-pushed the bugfix/WISHLISTS-25_fix-wishlist-api branch from 643113a to 592e365 Compare February 25, 2022 04:27
@auroraaaashen auroraaaashen force-pushed the bugfix/WISHLISTS-25_fix-wishlist-api branch from 592e365 to be48c2a Compare February 25, 2022 04:36
Copy link
Contributor

@btalma btalma left a comment

Choose a reason for hiding this comment

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

I would recommend combining FirstScroll and SubScroll into one function that uses the token if present otherwise sets size.

@@ -229,12 +240,13 @@ public async Task VerifySchema()
}
}

public async Task<WishListsWrapper> GetAllLists()
private async Task<string> firstScroll()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Pascal Case: FirstScroll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Brian, thank you for the comments.
I tried to run the first scroll call and all the sub calls in the postman and found that they all have the X-VTEX-MD-TOKEN in the header. The first sub call always has the same token as the initial call, and other sub calls' tokens can be updated by themselves sometimes.
So I think checking if the token is present is not a good idea since we want to make sure all sub calls use the token from the first scroll call.

I will update functions to Pascal Case.

dotnet/Data/WishListRepository.cs Outdated Show resolved Hide resolved
var response = await client.SendAsync(request);

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the token need to be reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the scroll document https://developers.vtex.com/vtex-rest-api/reference/scrolldocuments, all subsequent requests use the same token from the first call

@auroraaaashen auroraaaashen force-pushed the bugfix/WISHLISTS-25_fix-wishlist-api branch from 63a71a9 to a1f2be9 Compare February 25, 2022 15:53
@auroraaaashen auroraaaashen force-pushed the bugfix/WISHLISTS-25_fix-wishlist-api branch from a1f2be9 to 28dc87a Compare February 25, 2022 15:54
@sonarqubecloud
Copy link

[vtex-apps_wish-list-ts] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

[vtex-apps_wish-list-dotnet] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@auroraaaashen auroraaaashen merged commit 7df6f7f into master Feb 25, 2022
@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Feb 25, 2022

Your PR has been merged! App is being published. 🚀
Version 1.10.0 → 1.11.0

After the publishing process has been completed (check #vtex-io-releases) and doing A/B tests with the new version, you can deploy your release by running:

vtex deploy vtex.wish-list@1.11.0

After that your app will be updated on all accounts.

For more information on the deployment process check the docs. 📖

@auroraaaashen auroraaaashen deleted the bugfix/WISHLISTS-25_fix-wishlist-api branch August 2, 2022 21:24
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