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

A few Amazon downloader fixes #38

Merged
merged 1 commit into from
Mar 15, 2021
Merged

A few Amazon downloader fixes #38

merged 1 commit into from
Mar 15, 2021

Conversation

carljm
Copy link
Collaborator

@carljm carljm commented Jan 18, 2021

  1. I introduced an off-by-one error on the order groups index in Add order_groups option to Amazon scraper #32. This is the same issue fixed in Fix off by one error. #37, which I just now noticed.
  2. If the desired order group is already-selected, the wait_for_page_load on selecting that index fails, because nothing changes on selecting the already-selected option. So only actually select the option if it's not already selected.
  3. Digital order invoice downloading was broken because the print_url was wrong for digital invoices. The most robust way to handle this is just to use the link href instead of manually constructing a URL which may get out of date. This requires verifying that we actually have the Invoice link instead of another link that contains orderId in the href (e.g. "Order Details"). Currently I do this by checking the link text; I could instead do it by checking some contents of the href (though I'd have to check for different marker link contents for digital vs physical orders). Both approaches could be broken by website changes, but in this case looking for the text "Invoice" seems more reliable than relying on details of the URL format, which apparently already has changed once for digital invoices.

Verified that these changes work for downloading both physical and digital order invoices.

@carljm carljm mentioned this pull request Jan 18, 2021
@carljm
Copy link
Collaborator Author

carljm commented Mar 15, 2021

Hi @jbms, been using this for a while and it definitely fixes some significant problems in the Amazon importer. Any changes you'd like to see before merge?

@jbms jbms merged commit 659e820 into jbms:master Mar 15, 2021
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.

2 participants