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

fix(api): Use query params instead of body #139

Merged
merged 1 commit into from
May 2, 2023

Conversation

vladimirkosmala
Copy link
Contributor

@vladimirkosmala vladimirkosmala commented May 2, 2023

πŸ“‘ Description

The getVideoStatistics arguments were passed using body, but the Bunny API uses query string for this method.
The consequence was to have no argument passed to the API and return a global information (the whole library) instead of a specific video by its GUID.
The fix changes where the data are put: in the query string instead of the body.

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

The tests should have seen it but the cost to update the test for this case is high: it would require to upload 2 videos (easy) but also to simulate watch time (harder) to distinguish between the global stats and the specific stats which have both '0' values because no watch time is set.
To me it's not worth it.

@vladimirkosmala vladimirkosmala marked this pull request as ready for review May 2, 2023 18:55
@dan-online
Copy link
Owner

@vladimirkosmala Great catch, I agree with your thoughts on the tests, it's not feasible or necessary to attempt to add watch time or a similar statistic

@dan-online dan-online merged commit ba93b87 into dan-online:main May 2, 2023
2 of 3 checks passed
@vladimirkosmala vladimirkosmala deleted the fix/stats-call branch May 2, 2023 21:22
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.

None yet

2 participants