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

Query parameter time_filter ignored #49

Closed
HughbertD opened this issue Dec 5, 2018 · 6 comments · Fixed by #50
Closed

Query parameter time_filter ignored #49

HughbertD opened this issue Dec 5, 2018 · 6 comments · Fixed by #50
Labels

Comments

@HughbertD
Copy link

Current Behavior

When making a request to: /users/me/orders/?time_filter=current_future via the SDK I am receiving all the orders back regardless of the time_filter used

Expected Behavior

I expect to receive only the current_future or past events depending on the parameter I provide

Possible Solution

Steps to Reproduce (for bugs)

Generate a token
Provide a request to: /users/me/orders/?time_filter=current_future
Observe the orders returned

Provide a request to: /users/me/orders/?time_filter=past
Observe the same orders are returned

I then used Insomnia (API client) to make the same request (same token etc) and observed properly filtered events returned depending on the query parameter provided.

Screenshots (if appropriate):

Your Environment

  • SDK version: 1.0.3
  • Node version: 8.11.4
  • Browser name and version: Chrome
  • Operating system: Mac OS, El Capitan
@benmvp
Copy link
Contributor

benmvp commented Dec 6, 2018

Hi @HughbertD! 👋🏾

Thanks for your issue. Can you include two things for me?

  1. Sample code for how you're making the request
  2. A screenshot or other reproduction of the Network tab in your Chrome Developer Tools showing the request be made

This will help us figure out where the breakdown is happening.

@HughbertD
Copy link
Author

Hi @benmvp

Thanks for taking a look!

Request is being made in node on the backend, so I can't show you a network tab.

const sdk = eventbrite.default({token: req.session.token});
  sdk.request('/users/me/orders/?time_filter=current_future').then(res => {
    console.log(res.orders.length);
    console.log(res.orders);
  }).catch(error => {
    console.log('ERROR:');
    console.log(error);
  });

Similar request in Insomnia that bears different (and correct) results
https://cl.ly/342dc6f0bd02

@kwelch
Copy link
Contributor

kwelch commented Dec 6, 2018

I created a codesandbox that emulates the issue. I don't see why it would do that, but you can see from the sandbox that is does return 17 items, however when I use the API explorer it only returns 1.

Sandbox Result
image

API Explorer
image

UPDATE
I ran another test with a web based sandbox, and was able to get the network request. However, it has the params as I would have expected, but still returned all results.
image

@kwelch
Copy link
Contributor

kwelch commented Dec 7, 2018

I debugged this locally with the API. The SDK defaults the Content-Type to application/json. Within the API parses that Content-Type tells it to ignore the query params and use the data or body of the request.

A workaround is to specifically set the method of the request to GET, which skips out defaulting. I have a sample react app that does this here.

I will also open a PR that will default method to GET.

I also threw it in Netlify, since it was easy and shouldn't have to build to test it.
https://youthful-raman-d8ec00.netlify.com/

@HughbertD
Copy link
Author

Thanks @kwelch for looking into this, the work around is doing the trick for the time being :) 👍

kwelch-eb pushed a commit that referenced this issue Dec 27, 2018
Defaults request method to "GET" to avoid setting the "Content-Type" to json for requests where method is omitted.

PR: #50 

Resolves: #49
@ebtravis
Copy link
Collaborator

🎉 This issue has been resolved in version 1.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants