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

Filter naming is inconsistent and confusing. #42

Closed
mittonk opened this issue Aug 23, 2016 · 24 comments
Closed

Filter naming is inconsistent and confusing. #42

mittonk opened this issue Aug 23, 2016 · 24 comments
Assignees

Comments

@mittonk
Copy link
Contributor

mittonk commented Aug 23, 2016

Especially for Search: https://github.com/Yelp/yelp-api-v3/blob/master/docs/api-references/businesses-search.md

"open_now_filter" but "open_at"?

"pricing_filter" but "attributes"?

@mittonk
Copy link
Contributor Author

mittonk commented Aug 23, 2016

@tomelm , you mentioned you were changing these filter names --- assigning to you.

@tomelm
Copy link
Contributor

tomelm commented Aug 23, 2016

Code is live and documentation has been changed!

@tylerswartz
Copy link

@tomelm, is the code live for open_now? I sent the following request this morning and got back 39 results. I was expecting to get back 0 since it was so early in the morning. Any ideas why it doesn't appear to be filtering the response to only businesses that are open?

https://api.yelp.com/v3/businesses/search?term=drinks&latitude=37.7872562&longitude=-122.4418221&price_filter=1,2,3,4&radius=1800&limit=20&offset=0&categories=bars,beergardens&open_now=true

@tomelm
Copy link
Contributor

tomelm commented Aug 24, 2016

@tylerswartz yep, it's live. Let me dig into that a bit and see if I can figure out what's happening.

@mittonk
Copy link
Contributor Author

mittonk commented Aug 24, 2016

Repro'd: https://gist.github.com/mittonk/de9d58424ed28fa00279e3ca9e768ba1

Spot-checked those businesses, Social Study and Horsefeather are both listed "closed right now" on the www.yelp.com view.

@mittonk mittonk reopened this Aug 24, 2016
@tomelm
Copy link
Contributor

tomelm commented Aug 25, 2016

@tylerswartz we're seeing the problem too, right now I think it's something involving timezones. Trying to figure out how to tackle it but no eta at the moment. I'll post updates in this thread as I have them

@tylerswartz
Copy link

Thanks for the update, keep me posted.

@tomelm
Copy link
Contributor

tomelm commented Aug 25, 2016

@tylerswartz would it possible for you to use open_at but with the current time of day?

@tylerswartz
Copy link

tylerswartz commented Aug 25, 2016

@tomelm yes I can go that route. To confirm I should use the UNIX time format (ala 1095379198.75) for the local time zone of the user's search area. Is that correct?

While this requires a few more lines of code on my side this is a better solution since I'd prefer to know what business is open for another 30minutes from the current time anyways. I'll pull the current unix time and add ~30 minutes, then send it via open_at.

@tomelm
Copy link
Contributor

tomelm commented Aug 26, 2016

@tylerswartz that's right. I think I might be seeing some issue with open_at too, let me know if you're also seeing them

@tylerswartz
Copy link

yes @tomelm, open_at doesn't seem to work for me either.

This is the request I made this morning at 5:30am when no businesses are open, but it responded with everything open.

https://api.yelp.com/v3/businesses/search?term=drinks&latitude=37.33980485833911&longitude=-121.8868321746048&price=1,2,3,4&radius=4014&categories=bars,beergardens&open_at=1472559632

@tylerswartz
Copy link

Hi @tomelm - checking in on this. Do you have a status of the fix?

@zuchie
Copy link

zuchie commented Sep 4, 2016

For now a workaround is to use (Unix time + local seconds from UTC) as open_at time. I guess the API is using open_at comparing against business hours in UTC that caused this.

@zuchie
Copy link

zuchie commented Sep 4, 2016

If my guess was right, for open_at, without offsetting Unix time from users, one proposal for the fix from API side: timeToCompareWithOpenAt = unixTime(business.literalHours) - secondsFromUTC(business.timeZone). This should also work for open_now.

@tylerswartz
Copy link

Thanks @zuchie that work around does work.

I'm a little hesitant to roll this change out to my app because it's relying on Yelp's API using a weird localized version of GMT to check the business hours.

For example, if a business is open at 0500 PST, Yelp appears to have the business hours recorded as 0500 GMT. If they were to correctly update the system to identify that the local business hours of 0500 PST is actually 1200 GMT then the suggested workaround would break.

Here is how I could potentially implement the workaround. Would much rather have a way to send the current UNIX time (as @tomelm originally suggested) instead of this offset localized version.

//get current unix time in UTC
let currentUTCTime = Int(NSDate().timeIntervalSince1970)

//find local seconds from UTC
let secondsFromGMT = Int(NSTimeZone.localTimeZone().secondsFromGMT)

//offset UTC time by local seconds
let openTime = currentUTCTime + secondsFromGMT

let open_at = openTime
print(open_at)

@zuchie
Copy link

zuchie commented Sep 5, 2016

@tylerswartz way to go for open_at for now, the exact same here. You are right, if API got changed this workaround would break definitely. I think the once and for all solution is the API to change the way the business hours from GMT to local timezone, as I also suggested in my last post, otherwise the open_now won't be working anyway.

@zuchie
Copy link

zuchie commented Sep 6, 2016

@tylerswartz better yet for the open_at workaround, use secondsFromGMT of a given location instead of NSTimeZone.localTimeZone() which gives default timezone of current app, so that when the given location isn't in the same timezone with the app it'll still give correct open_at value. Also need to take daylight-savings time into consideration.

For example, query a business in NY, open_at = app's Unix timestamp + offset for daylight-savings(3600) + offset of business from UTC(-18000)

I'm using Google Time Zone API to get this done.

@tylerswartz
Copy link

Thanks @zuchie, I'll make that change. Still, I'd prefer a solution recommended by @tomelm since this relies on Yelp to not update the timezone for businesses in their db. 😃

@tomelm
Copy link
Contributor

tomelm commented Sep 6, 2016

Hey @tylerswartz @zuchie! I've got a fix on our side that I'm trying to push out soon. The issue does fall back to timezones, a businesses open hours is stored in its local timezone. It should work where if you provide us the time you want to check it's open at in UTC (regardless of the business's location or timezone), we'll correctly handle the rest. I'll keep you both posted on this fix

@tylerswartz
Copy link

Awesome, thanks @tomelm, I appreciate your work on this!

@tomelm
Copy link
Contributor

tomelm commented Sep 7, 2016

The change for this should be out. Do you want to try open_at again? Try sending over the open_at time in UTC

@zuchie
Copy link

zuchie commented Sep 7, 2016

It's working now sending UTC to open_at, tested with businesses at timezone PDT or EDT, correct open/closed status of the queries had been returned. Thanks @tomelm, this will save me the work of converting from UTC to local time.

Just curious, is it that the API was using UTC(business hours) before, and now it's localTime(business hours) after the fix?

@tylerswartz
Copy link

Great @tomelm it is working as expected. Thanks!

@tomelm
Copy link
Contributor

tomelm commented Sep 7, 2016

Great, glad it's all set! Let me know if there are any other problems

@zuchie yep, that was it.

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

No branches or pull requests

4 participants