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

[Amazon] Multi-Language Support #50

Merged
merged 2 commits into from
Mar 6, 2022
Merged

[Amazon] Multi-Language Support #50

merged 2 commits into from
Mar 6, 2022

Conversation

moritzj29
Copy link
Contributor

@moritzj29 moritzj29 commented Jan 12, 2022

Hello,

I took PR #48 by @Marvin182 and addressed the comments of @Zburatorul from the code review to make this very valuable contribution mergeable:

  • include upstream code changes (Amazon fresh)
  • revert back to the old way of looking for invoice links by looking for orderID

I tested this with my amazon.de account and it works very well. All credit goes out to @Marvin182 for implementing this in the first place!

I'm happy for your comments!

PS: I'm also working on the corresponding beancount-import module over here: jbms/beancount-import#144

@moritzj29
Copy link
Contributor Author

found some flaws... I will get back working on this...

@moritzj29
Copy link
Contributor Author

some background to better understand the changes in the code (I find this quite helpful since we all only know our "own" amazon pages...):

From the code I assume that for amazon.com/.uk the orders overview look something like this with the Invoice button linking directly to the order_summary which we want to download:
Amazon

For amazon.de the page is different, since Invoice always opens a menu. This menu is loaded on demand, therefore a click is required to retrieve the link. (This is probably due to the fact that invoices need to be in an unalterable format like PDF in Germany, so HTML is not sufficient as official invoice.)
Amazon

In the code I added an additional per-domain flag, indicating if the order_summary link is hidden in the submenu or not. Depending on the flag, the link is retrieved the old way (master branch) or the new way as proposed in #48.

It would be great if someone with a .com or .uk account could test the code to make sure everything works out.

@moritzj29 moritzj29 marked this pull request as ready for review January 13, 2022 05:39
self.regular = regular
self.digital = digital if digital is not None else default_digital
self.digital_orders_menu = digital if digital is not None else self.domain.digital_orders_menu
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed this to make it more clear that is does not toggle the download of digital invoices in all cases (only for .com!). It's basically a flag which says if there is a separate digital orders list on the amazon website or not.

@moritzj29
Copy link
Contributor Author

I updated the PR to include the upstream changes to the Amazon source and cleaned up the history. It would be great if someone could have a look at it! @Zburatorul you appear to be most involved at the moment?

If there is no interest in merging this here, just tell me. I will maintain it in my own fork then.

@jbms jbms merged commit 83b58ad into jbms:master Mar 6, 2022
@moritzj29
Copy link
Contributor Author

awesome! thanks @jbms !

@Zburatorul
Copy link
Collaborator

@Marvin182, @moritzj29, thanks for the contributions and for getting this over the line!

@Zburatorul
Copy link
Collaborator

@moritzj29, there's a bug for digital orders on line 471. Undefined variable.

@moritzj29
Copy link
Contributor Author

Oh sorry,self.domain.digital_orders_text must be self.domain.digital_orders_menu_text . I will submit a fix today. Unfortunately I had no possibility to test run it on .com…

# different labels are possible e.g. for regular orders vs. Amazon fresh
if invoice_link.text != "":
# log non-empty link texts -> may be new type
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you turn this into a logger.debug instead please. It prints on every single order and pollutes the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sure

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.

3 participants