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

[14.0][MIG] pos_payment_terminal #572

Merged
merged 79 commits into from
Feb 18, 2021

Conversation

alexis-via
Copy link
Contributor

This PR works and has been tested on real Ingenico card reader (for France, with Telium) connected via pywebdriver (master branch).

It is a full re-write of the module, in order to take advantage of the new payment interface JS framework of Odoo POS.

FR translation updated.

Auélien DUMAINE and others added 30 commits December 16, 2020 22:06
…ixing unit_price computation on adding orderLine
pos_customer_display: FIX JS code and make it more robust
Adapt JS code of pos_payment_terminal
Some cleanup
…tion !)

Move CSS definition from pos_payment_terminal_view.xml to pos_payment_terminal.xml
Small cleanups
In order to get visibility on https://www.odoo.com/apps the OCA board has
decided to add the OCA as author of all the addons maintained as part of the
association.
… to new API

pos_payment_terminal: code cleanup
Move description from __openerp__.py to README.rst
Update demo data
…debase to properly get the 'name' of the currency
@sbidoul
Copy link
Member

sbidoul commented Feb 16, 2021

@alexis-via this is not finished, right ?

This module needs to support send_payment_cancel and send_payment_request must return a promise that manages the transaction status via the change:status event and the payment transactions list we get in it.

Are you or @PierrickBrun working on this these days ? Shall we coordinate or efforts ?

Have send_payment_request return a promise as requested by the
documentation. This open the door to handling the transaction status
reported by the terminal driver asynchronously.

Also, catch errors when sending the transaction start command.
@sbidoul
Copy link
Member

sbidoul commented Feb 17, 2021

@alexis-via I made this PR green in akretion#20 and improved it a little bit.

Now I'll work on handling the transaction statuses from the payment terminal drivers, compatible with pywebdriver.

[14.0] pos_payment_terminal migration improvements
@sbidoul
Copy link
Member

sbidoul commented Feb 17, 2021

@ivantodorovich @legalsylvain your review comments have been processed.

@sbidoul
Copy link
Member

sbidoul commented Feb 17, 2021

BTW, when the transaction has been sent to the terminal, this is what happens in the POS - I post this so others don't have to research why this is normal :)

image

This is normal behaviour until we have transaction status from the driver: the user has to manually confirm that the transaction suceeded by clicking "Force done". The "Connection error" message is hard coded in Odoo.

Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Please squash migration commits before merging

@sbidoul
Copy link
Member

sbidoul commented Feb 17, 2021

Please squash migration commits before merging

Well, there are two authors and this is more a rewrite than a migration, and the commit history helps understand why things are as they are.

@ivantodorovich
Copy link
Contributor

ivantodorovich commented Feb 17, 2021

Please squash migration commits before merging

Well, there are two authors and this is more a rewrite than a migration, and the commit history helps understand why things are as they are.

I'd at least clean a bit the commits, whilst keeping the authors

For instance there are commits like

  • pos_payment_terminal: remove commented lines,
  • pos_payment_terminal: remove dead code ,
  • pos_payment_terminal: fix lint issues,
  • pos_payment_terminal: black, isort and pre-commit stuff

It's also a good opportunity to clean the previous history.
There are things like this that could be squashed:

image

as well as the usual administrative commits

Squash administrative commits (if any) with the previous commit for reducing commit noise. They are named as "[UPD] README.rst", "[UPD] Update $MODULE.pot", "Update translation files" and similar names, and comes from OCA-git-bot, oca-travis or oca-transbot. IMPORTANT: Don't squash legit translation commits, authored by their translators, with the message "Translated using Weblate (...)".

https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sbidoul
Copy link
Member

sbidoul commented Feb 17, 2021

I try to make small commits that are easy to review, and also sufficiently atomic that they are easy to revert. I also strive to not rewrite past history. Anyway, I can't do the squashing myself here as this is not a branch I own.

@ivantodorovich
Copy link
Contributor

I'm not blocking in this 👍🏻

@alexis-via
Copy link
Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-572-by-alexis-via-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit aeaf804 into OCA:14.0 Feb 18, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at c7e3587. Thanks a lot for contributing to OCA. ❤️

@chienandalu chienandalu mentioned this pull request Apr 26, 2021
16 tasks
@chienandalu chienandalu added this to the 14.0 milestone Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.