-
Notifications
You must be signed in to change notification settings - Fork 38
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
Amazon: Special logic for Amazon Fresh orders #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will review this once mypy fix has been merged and this is rebased on top of it, so we can verify full build is green.
faa11fa
to
5ca428b
Compare
@@ -169,7 +169,8 @@ def invoice_finder(): | |||
|
|||
order_ids = set() | |||
for invoice_link in invoices: | |||
if "nvoice" not in invoice_link.text: | |||
# Amazon Fresh, and regular orders respectively | |||
if invoice_link.text not in ("View order", "View invoice"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't recall why I found this change necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's clear in the new approach why it's required :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, tox is green now and the PR is ready for review @carljm
finance_dl/amazon.py
Outdated
@@ -264,8 +265,22 @@ def get_source(): | |||
source = self.driver.page_source | |||
if 'Grand Total:' in source: | |||
return source | |||
if 'Final Details for Order' not in source: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the logic can be confusing.
Previously get_source
returned the source when it identified it was on an invoice.
Now, in addition to doing that, it will return the source when it identifies that it is on an intermediate pre-invoice page that Amazon Fresh orders lead to from the Orders page.
By returning the source it allows for another retry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than checking that "Final Details for Order" is not on the page (which could indicate various kinds of failure), is there some reliable identifier on the intermediate Amazon Fresh pages that we can use to positively identify that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I found some a more Fresh-specific identifier. No idea about reliability though as it's a throwaway phrase they could easily reword/remove in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with this as-is, though I think it might be better to more explicitly positively identify the Amazon Fresh interstitial page.
5ca428b
to
b046421
Compare
to invoice on the Orders page.
b046421
to
39402c6
Compare
Sorry about the numerous changes Carl. I finally saw how to do this in a simpler way without muddying the waters in multiple methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! Looks good.
@@ -169,7 +169,8 @@ def invoice_finder(): | |||
|
|||
order_ids = set() | |||
for invoice_link in invoices: | |||
if "nvoice" not in invoice_link.text: | |||
# Amazon Fresh, and regular orders respectively | |||
if invoice_link.text not in ("View order", "View invoice"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's clear in the new approach why it's required :)
Amazon Fresh orders lack a direct link to the invoice on the Orders page. One must go one page deeper.
In some cases that fails to, and one can fall back on constructing the invoice URL.