search

Revadike / epicgames-freebies-claimer

Claim available free game promotions from the Epic Games Store.
MIT License
2.23k stars 359 forks source link

Fix promos filter #52

Closed davispuh closed 4 years ago

davispuh commented 4 years ago

Currently Hitman is free with 100% discount https://www.epicgames.com/store/en-US/product/hitman/standard-edition But it was incorrectly filtered out and wasn't tried to claim it. This PR fixes that items won't be wrongly filtered out.

Revadike commented 4 years ago

Why are you getting rid of the product/bundle distinction? This was relevant in the week GTA V was free. It may have changed, but the returned data on free promotions differ weekly and it should support it every week. Let me know how confident you are this code will support other weeks too.

davispuh commented 4 years ago

Current promos are

This PR works for both, there's no need for any such transformation. They return correct offer namespace and id but unfortunately I don't know how freeGamesPromotions looked like in past/previous weeks and we don't have any tests so we can't really know.

So I would say I'll add tests for this weeks case and then if next time it doesn't work we fix it again and add another test case.

Revadike commented 4 years ago

Well, that is what I was saying. It was adjusted for those previous weeks and this PR seems only concerned with this week's format.

davispuh commented 4 years ago

Yeah, current code doesn't work for this week. Maybe they had bug previously and that's why you had to do these extra steps but to me it looks like you shouldn't need them.

If I knew what output looked like then and what was failing then could see best solution to fix it. As for now only option is either remove like I do here and fix next time if it doesn't work or add extra logic for this case.

Revadike commented 4 years ago

Actually, I think this provides an up to date solution filter.

davispuh commented 4 years ago

Hadn't seen that project before, but it looks really good, way better than this one. And that implementation is exactly how I imaged it should be https://github.com/claabs/epicgames-freegames-node/blob/master/src/free-games.ts#L145

The only downside of that project is that it's duplicating a lot of node-epicgames-client. So I would say should extract that functionality out of it, make it depend on epicgames-client and use that one instead of maintaining this one. So I think that's what I'll be doing :D

davispuh commented 4 years ago

Btw see claabs/epicgames-freegames-node/issues/33

Revadike commented 4 years ago

I think his project is a bit more advanced for a regular user, while my project is a bit more simplistic.

Revadike commented 4 years ago

Also, please check his updateIds, where for example he distinguishes bundle from product, which my solution was doing too, but which you removed now.

davispuh commented 4 years ago

I see, but I still don't get why it's needed. Like it shouldn't be. freeGamesPromotions returns offer id and namespace. Anyway I updated this PR to work for how it was previously and fix for this weeks. Also added tests so now next time will see if there's any difference.

davispuh commented 4 years ago

I think his project is a bit more advanced for a regular user, while my project is a bit more simplistic.

In what way? Like to me sending email when asked for captcha is very important. I'm planning on running it headless on server. So it has features that people would need. Also setup should be pretty simple for both. It always can be easily fixed to be simple to install and use.