ssokolow / itad_importer

A userscript helper for importing game collections into IsThereAnyDeal.com
https://greasyfork.org/en/scripts/13887-isthereanydeal-com-collection-importer
MIT License
83 stars 8 forks source link

Import not working due to 404 error #28

Closed pizzahut2 closed 6 years ago

pizzahut2 commented 6 years ago

I was trying to import my GOG collection, and ended up with a 404 error page:

https://isthereanydeal.com/outside/user/collection/3rdparty

"404 This page doesn't exists" [sic]

ssokolow commented 6 years ago

Ugh. Looks like the recent API update removed the endpoint I rely on.

@tfedor Is there an easy "submit to a different URL and maybe rename some of the submitted fields" fix for this or is my extension dead until I have time for the very backlogged complete rewrite?

tfedor commented 6 years ago

Yeah, it's completely removed, I had changed the way it works both frontend and backend. Which reminds me I didn't get back to you via email, sorry abou that :(

Fix shouldn't be hard though, since you already have parsers done, you just need to update the format you are sending and URL.

This is the minimal JSON you need to send for Collection import:

{
    "version": "02",
    "data": [{
        "title": "Oxygen Not Included",
        "copies": [
            {
                "type": "steam"
            }
        ]
    }]
}

Optionally you may add additional info, complete example is in docs: https://itad.docs.apiary.io/#reference/collection/import-via-form You may also set custom type, it doesn't have to be store id anymore.

You will send the request here: https://api.isthereanydeal.com/collection/import/ with a POST body:

The body of the request will simulate form submission. It has to contain two fields: file and upload. The content of file is base64 encoded JSON string, the content of upload field is not important.

Import to Waitlist is basically the same, with slightly different format and URL.

I promise this won't change anytime soon, I tried to make it robust so if I need to change the format I can support older versions as well (hence version in the JSON). The old solution was hacked together and not really maintainable.

ssokolow commented 6 years ago

Sorry for being silent for almost three weeks. Life's been a mess and I've had other obligations that took priority. I'll try to get this script fixed up within the next few days.

tfedor commented 6 years ago

If you need anything from me to get it working let me know, I'll make it my priority.

tfedor commented 6 years ago

Hey Stephan, did you have a chance to take a look at new API yet?

ssokolow commented 6 years ago

Things kept coming up and this got pushed down the stack. I'm really exhausted right now, but I'll try to get around to it within the next couple of days.

EDIT: Next few days. Various things came up that had to be prioritized. (That said, I am keeping this at the top of the stack so I'll get to it as soon as possible.)

ssokolow commented 6 years ago

Sorry for letting a couple of days turn into a couple of weeks. I'm just finishing wrestling down the unexpected pile of things that had to come first, so I hope to have a fix for this soon.

rmenessec commented 6 years ago

Fingers crossed. This script appears to be the only way to sync GOG—well, most of the supported sites—to ITAD.😒 Thanks for your work, @ssokolow.

ssokolow commented 6 years ago

The ITAD developers can't really do anything inside ITAD itself because sites like GOG don't allow arbitrary sites to access user data.

Also, sorry for again letting things drag on. Just when I was getting close to being able to work on this almost three weeks ago, things got busy again.

No promises, but I've clawed my way back, so here's hoping I'll have it fixed some time within the next few days. (I really need to find time to write a test suite for this so changes that should be trivial are trivial.)

rmenessec commented 6 years ago

@ssokolow, I wonder. Is it possible to get game lists via the GOG Galaxy APIs?

ssokolow commented 6 years ago

The problem is credentials. As far as I know, there's no way to authorize ITAD to just get a game list without giving full permissions to your account... at which point, it's not much better than giving ITAD your username and password so it can log into the web interface and scrape the library view.

ssokolow commented 6 years ago

I've started work on this, but I was up late last night (got really into a good story) and woke at the usual time, so I'm quite dozy. I anticipate that, by the time you guys read this, I'll have everything done except double-checking that I remember the caveats of doing AJAX POST data.

ssokolow commented 6 years ago

Never mind. My first test attempt resulted in a 500 Internal Server Error from ITAD.

I can't debug that from this end.

tfedor commented 6 years ago

Checking now, we moved servers like 3 days ago, so we probably broke something

ssokolow commented 6 years ago

...and I just realized what part of it may be. My tired mind didn't notice that I'd failed to properly translate the old copies to the new sources while adjusting the JSON template.

tfedor commented 6 years ago

Did that help?

ssokolow commented 6 years ago

At the moment, I'm trying to figure out what this nonsense "Colon assignment without proper spacing" error is that CoffeeScript is throwing at me. I'm definitely switching over to TypeScript once I've got it working again.

EDIT: Figured it out. I had something set to turn warnings into errors and Syntastic was only showing the first line of the message so my mind was running down a false track. (That's what 4-5 hours of sleep will do to you sometimes.)

ssokolow commented 6 years ago

Nope. Apparently still an Error 500.

tfedor commented 6 years ago

Alright, can you send me the request you are making? I'll check on my end

tfedor commented 6 years ago

Ok, I think I see it, just to check it, are you logged in or logged out on ITAD?

ssokolow commented 6 years ago

That could be it. I forgot that ITAD logs me out after a while.

tfedor commented 6 years ago

Yeah, seems like I'm not properly checking that. It should work when you log in and I'll do a hotfix in the meantime

ssokolow commented 6 years ago

OK.

Now I'm getting "Uploaded data couldn't be parsed, corrupted file?", which is exactly what I was expecting. (Step 1: Make sure I'm hitting the right endpoint. Step 2: Debug my first attempt at following the new schema.)

tfedor commented 6 years ago

And it shouldn't do 500 when user is not logged in anymore.

ssokolow commented 6 years ago

Hmm. I'm still getting the corrupted file error. At what point in the process does that get returned?

Should I be checking that the <form> I inject and then submit is configured properly? Should I be checking that I don't have any extra form fields left kicking around from the old schema? ...JSON fields? Will it complain if a scraper fails and injects a null value where a differen type is expected? Something else?

tfedor commented 6 years ago

Extra fields shouldn't be problem. Corrupted Data can be returned if:

  1. json itself is not valid
  2. there's missing version or data field
  3. if title of the game is empty or missing, if copies are empty or missing or if copy doesn't have type

I'll give you better error message in few minutes

tfedor commented 6 years ago

Error message should give you a reason now so you at least know where to look at.

ssokolow commented 6 years ago

Thanks. I know what went wrong now... I forgot that I was still dumping un-base64-encoded stringified JSON into a hidden field.

I may defer the fix for that until tomorrow in favour of something else like debugging the Humble Bundle export button failing to show up since I need to catch up on options for programmatically generating file uploads when the request should also navigate the top-level frame.

(Last time I needed to do that, it was a bit of a messy process to do those two things together when you didn't have full freedom to modify what the receiving end expected.)

sorsasampo commented 6 years ago

@tfedor now would be a good time to write integration tests for the cases you guys ran into, it will pay itself back in the long run :)

(and speaking of copies, it would be nice if ITAD export sorted them)

ssokolow commented 6 years ago

The hard part involved in writing integration tests is getting the framework set up to write tests at all. The things I haven't yet pinned down a good answer for are:

  1. How to set up an automated test harness for a Greasemonkey script.
  2. Testing against ITAD (Does ITAD have a test server that would be suitable for CI use?)
  3. Best practices for testing JavaScript code which scrapes DOMs where the live version isn't visible with the credentials available to the test server and I won't get notified of changes by the developers of the live version.

Once I've got answers for those, I definitely want to put together a test suite.

ssokolow commented 6 years ago

I got the submission encoding fixed up as far as I can tell (non-ASCII characters like ® are translating through properly), but the "already in collection" detection seems to be broken as everything in my test GOG import is checked, including stuff that I can confirm to have already been added using the old API.

Also, "Remove from Waitlist" isn't defaulting to checked anymore and I don't see an API field to change that.

tfedor commented 6 years ago

I think it's supposed to be checked, but if you click import instead of replace it won't have any effect (it should be skipped during that step).

Remove from Waitlist shoudl be defaulting to https://isthereanydeal.com/settings/collection/?a=syncCollectionRemove if I remember correctly.

ssokolow commented 6 years ago

I think it's supposed to be checked, but if you click import instead of replace it won't have any effect (it should be skipped during that step).

Apparently that's not what your past self intended, because there's an info banner at the top of the page which says otherwise.

Games that you already have in Collection are by default not selected

(And even if it were behaving as intended, i'd still report it as a regression in functionality I rely on and a loss of intuitiveness.)

Remove from Waitlist shoudl be defaulting to https://isthereanydeal.com/settings/collection/?a=syncCollectionRemove if I remember correctly.

"Automatically remove games from my Waitlist when they are added to Collection" is checked in my settings but "Remove from Waitlist" defaults to unchecked in the import confirmation page.

tfedor commented 6 years ago

"Automatically remove games from my Waitlist when they are added to Collection" is checked in my settings but "Remove from Waitlist" defaults to unchecked in the import confirmation page.

This is working for me correctly.

Regarding default selection I'm not seeing any problem in code, but if I won't see it in few minutes I'll continue tomorrow. I don't trust myself with code right now to be honest.

tfedor commented 6 years ago

Yup, from what I can see from code the note is wrong, apparently I changed my mind at some point and didn't remove the note. Which makes sense for me since there is an import and replace option so if you want to replace all copies it's better to have everything selected by default. I'll look at it once again tomorrow (and remove the note if I'm correct), but I'm pretty sure that's the case.

ssokolow commented 6 years ago

OK, then I guess I'll have to file a bug report about it in the tracker.

I don't trust the "Import" button without some visible indication that my scraper has produced data that satisfies your deduplication routines because you have no global Undo button I can use to easily revert an accidental "flood my collection with duplicate entries" operation.

EDIT: (ie. If I see only checked entries in the confirmation screen and it's not my first import from the site in question, I assume something is broken. Period.)

ssokolow commented 6 years ago

Also, is the "owned elsewhere" notification still implemented? I quite liked that feature and it seems like something else you'd throw out if you threw out the "Games that you already have in Collection are by default not selected".

tfedor commented 6 years ago

I made some changes. Yes, games are still all checked by default, but I've added two icons that show you whether the game/copy is in your Collection or not. I don't think existence of game in Collection is good enough reason to make checkbox unchecked, moreover when it all depends on copies (what if you have one of the imported copy and not the other?). You have two import options anyway to determine what to do.

So now, if game itself is in a Collection it will show a book icon and when copy, checked by type, is in your Collection it will show checkmark. I've also updated the note. This should cover all cases I believe.

ssokolow commented 6 years ago

I'm perfectly fine with that... but, note or not, it's best practice to give every indicator icon a tooltip. First time I tested it, I actually glanced over the note and went straight to hovering my mouse on the icons.

Here are the tooltips I'd suggest for maximum clarity: Book: "name of game" is already in your collection. Checkmark: A copy of "name of game" from "name of store" is already in your collection.

ssokolow commented 6 years ago

Also I'm still seeing two waitlist-related bugs in the import confirmation list:

  1. "Remove from Waitlist" is still defaulting to unchecked despite being set in my settings.
  2. I'm not getting any kind of "waitlisted" indication on purchased-but-yet-to-be-imported GOG games that I waitlisted for testing purposes.
tfedor commented 6 years ago

There should be tooltips in Collection import now (via title attribute)

tfedor commented 6 years ago
  1. Remove from Waitlist I'm not able to replicate, it works for me when changing this setting as I would expect https://isthereanydeal.com/settings/collection/?a=syncWaitlistRemove

  2. I don't think this is neccessary to be honest

ssokolow commented 6 years ago

There should be tooltips in Collection import now (via title attribute)

I see the tooltips, but, given how small the icons are relative to the height of the cursor, the text-selection cursor makes it a bit vague as to which icon is being hovered over, since it's not obvious which pixel on the cursor is the hot spot.

I'd suggest setting cursor: default on the icons or perhaps even cursor: help as a supplementary cue that a tooltip is available.

Perhaps bound to a .fa[title] rule.

I don't think this is neccessary to be honest

I have certain exceptional games where I need it to remind me to UNcheck "remove from waitlist" because I own something in one collection but, instead of the usual "owned on any site" policy, I want to watch for an opportunity to re-buy on a specific site.

Without that indication, to feel safe in the integrity of my data, I'd have to maintain a second waitlist independent from the one that could be incorrectly pruned by my collection import.

Finally, I can't seem to remove this duplicate copy I introduced while testing. Clicking "Remove Copy" just causes the throbber to show up for a while and then vanish without anything changing.

tfedor commented 6 years ago

For your use case it might be better to just do not check Remove from Waitlist and remove games from Collection after sync: https://isthereanydeal.com/collection/#/filter:&wait There's a lot of info already, I don't want to introduce any more unneccessary clutter.

For duplicate copy removal, try now, there was a long running db query and it may have locked that row. If it's still not working I will investigate further.

ssokolow commented 6 years ago

For your use case it might be better to just do not check Remove from Waitlist and remove games from Collection after sync: https://isthereanydeal.com/collection/#/filter:&wait There's a lot of info already, I don't want to introduce any more unneccessary clutter.

Is there a problem I'm missing in putting the word "Waitlisted" above the date on the right end of the entry as in this mockup?

screenshot2

If anything, given how much stuff is packed into the left end of each row, that space on the right looks too empty.

If I understand the design correctly, that column is used to show the added date associated with each copy, so that top row should be unused. Failing that, would an icon like fa-bell or fa-envelope next to whatever goes there really be significantly more clutter? (I suggest fa-envelope for consistency with the notification icon in the header.)

It even makes a certain degree of intuitive sense, in that something being on the waitlist is to collection import as a deposit to reserve a copy is to purchasing.

For duplicate copy removal, try now, there was a long running db query and it may have locked that row. If it's still not working I will investigate further.

I don't see any change. According to the network panel in developer tools, it waited 80662ms and then returned an error 500.

tfedor commented 6 years ago

Waitlisted as you showed it would introduce yet another different style, which we have way too many already. I'll think of adding a tag like I'm using everywhere else, but still I don't think it's necessary and that your use case is just very unusual. Since there are other people following this thread, I believe, can I ask for opinion?

Regarding copy removal, can you try now? If it still happens, is it just for that copy or are other copies affected as well?

ssokolow commented 6 years ago

Waitlisted as you showed it would introduce yet another different style, which we have way too many already.

In what sense? How the "waitlisted" indication looks is just an idea. (it's the position that's important to the mockup). As for the position, if you're trying to share the template with another page on the site, point me at it and I'll see if I can think of a more compatible way to do things.

Regarding copy removal, can you try now? If it still happens, is it just for that copy or are other copies affected as well?

It worked this time.

tfedor commented 6 years ago

I've added waitlist tags to import and I hope everyone's finally happy with it now...

grnassar commented 6 years ago

I was just about to comment on that topic, due to the request for comments. I know anecdote is anecdotal, but I personally also find utility in a "waitlisted" indicator for items I plan to rebuy on another site or in a fashion that isn't otherwise already differentiated on ITAD. Thank you for that!

ssokolow commented 6 years ago

It meets my needs now.

As a UX guy, I think it still has two issues with learnability, but that's it.

  1. The aforementioned issue with having a text cursor for two icons small enough that it can be unclear which one is being hovered over due to the position of the hot pixel with text cursors being non-obvious. (As opposed to default or help where it's clearly at the tip of the arrow.)
  2. "Waiting" is less intuitive than "waitlisted". Am I waiting on the game or does "waiting" mean the ITAD backend is waiting for something and some of its internal data may be stale? This could be resolved with a tooltip and a help cursor to announce the tooltip's presence on a non-icon, but it'd be much simpler to just say "waitlisted" instead.

Since "working for GOG.com and Flying Bundle, Groupees and FireFlower Games untested, and Humble still broken" is better than "working for no URLs", I'll upload what progress I've made so far on the userscript.