pos icon indicating copy to clipboard operation
pos copied to clipboard

[ADD] 12.0 pos_mail_receipt module

Open PierrickBrun opened this issue 4 years ago • 34 comments

PierrickBrun avatar Dec 17 '19 08:12 PierrickBrun

Is this a backport of v13 functionality?

I did not know v13 included this. So this is not a backport.

This module allows you to send an e-mail to a customer without creating a partner, this is better to not force it to be more RGPD compliant. Also in this module you send the ticket after validating the payment and not before, which is more user friendly.

PierrickBrun avatar Dec 17 '19 14:12 PierrickBrun

Thank you for this module !

  • CSS for the email input should be changed a little to be centered

margin-left: 50px;

  • I didn't received mail from runbot (maybe that's normal ?)

  • And it was not clear if the mail was sent after clicking "OK". A green warming message "The email was sent" would be nice !

quentinDupont avatar Dec 17 '19 16:12 quentinDupont

I "fixed" the textinput

You can see the failed sent mails in Settings -> Technical -> Email -> Emails (you need to be in debug mode)

I'll change the button color once clicked to avoid having to handle notifications (unless it is easy but I don't know how)

PierrickBrun avatar Dec 17 '19 16:12 PierrickBrun

@legalsylvain I've addressed the problems you saw and added tests

PierrickBrun avatar Dec 19 '19 16:12 PierrickBrun

@legalsylvain @quentinDupont I think this is in good shape now. I had trouble applying good CSS to the PDF generated by the backend, but in the end I got it to work

PierrickBrun avatar Jan 31 '20 09:01 PierrickBrun

@PierrickBrun Could you explain why the force=True option is used by default when the mail is sent, and when the create_from_ui() with force=False is used? I'm worried the session could be hindered when the email can't be sent.

mclaeysb avatar Apr 17 '20 17:04 mclaeysb

@PierrickBrun Could you explain why the force=True option is used by default when the mail is sent, and when the create_from_ui() with force=False is used? I'm worried the session could be hindered when the email can't be sent.

When the request is immediately sent (ie. The PoS is connected to the server), there is a force=True to ensure the e-mail is sent right away. This is useful because you may need it to prove you did not steal or to have a free parking for instance. In that case, since the record is already created when we decide to send the email or not, the email is created from a separated endpoint.

When the request is sent afterwards (ie. The PoS gets back connection) there is no force=True because in that case it does not matter to send the email quickly. In that case the email is sent with the same request as the record creation, in the create_from_ui

PierrickBrun avatar Apr 20 '20 07:04 PierrickBrun

credit to @vvrossem for helping with this find!

mclaeysb avatar Apr 23 '20 13:04 mclaeysb

Hello all, Hello @mclaeysb and @PierrickBrun I tested the last version on one of our test database.

  1. Send ticket to a customer who has an email adress. I clicked on the send receipt button, which became green. The email was sent correctly.

I configured some text on the header/footer in the pos config. Le sent ticket in attachment contains the modifications.

  1. Selling without selecting any customer. When I click on the button to sent the ticket, a pop up opens to indicate an email adress. The email was sent correclty.

Something somewhat surpising : it is possible to re-click on the button, and enter a new email adress. But no ticket will be sent again. The best would be to prevent the user to click again on the button. But still we can live with that at the moment.

  1. I cut the Wifi and made a sale. I was able to click on the button to sent the receipt. There was no error. The wifi signal was red with the number 1 (to inform there is 1 non synchronised sale). I re-activated the Wifi. I clicked on the 1 number. Everything was synchronised and the email was in the queue.

=> for me everyting works fine.

vdewulf avatar May 12 '20 08:05 vdewulf

We need to upload it to PyPi. Is it ok? https://github.com/coopiteasy/pos/pull/36 @mclaeysb

cesarlr avatar Oct 20 '20 08:10 cesarlr

Hi @cesarlr! For this PR we are waiting for the OCA (@legalsylvain @chienandalu ?) to consider it :)

mclaeysb avatar Oct 20 '20 16:10 mclaeysb

some rebase should be done on this branch. (conflict, a lot of file modified, ...)

legalsylvain avatar Jan 21 '21 15:01 legalsylvain

The branch is still messy unfortunately. Make sure you push with --force after rebasing. Ideally, for this PR, only 1 commit should be here: [ADD] pos_mail_receipt

ivantodorovich avatar Jul 22 '21 18:07 ivantodorovich

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

github-actions[bot] avatar Jan 16 '22 12:01 github-actions[bot]

Hi @PierrickBrun , any chance to make a push rebase so that everyone could benefit from the huge amount of energy you spent on this?

Regards

flotho avatar Jan 17 '22 07:01 flotho

Hi @flotho , done. I quickly tested it and it looks like nothing's been broken in the process

PierrickBrun avatar Jan 17 '22 10:01 PierrickBrun

@legalsylvain This won't merge because travis is failing.

carmenbianca avatar May 23 '22 09:05 carmenbianca

@PierrickBrun : could you take a look on travis ?

/ocabot rebase

legalsylvain avatar May 23 '22 09:05 legalsylvain

@legalsylvain The rebase process failed, because command git push --force coopiteasy tmp-pr-428:12-pos_mail_receipt failed with output:

remote: Permission to coopiteasy/pos.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/coopiteasy/pos/': The requested URL returned error: 403

OCA-git-bot avatar May 23 '22 09:05 OCA-git-bot

@legalsylvain I'll rebase.

carmenbianca avatar May 23 '22 09:05 carmenbianca

still red !

legalsylvain avatar May 23 '22 10:05 legalsylvain

@legalsylvain The travis CI is red because of a timeout. Do you have an idea on how to fix this ?

victor-champonnois avatar Jun 23 '22 09:06 victor-champonnois

@victor-champonnois : not sure that is a time out problem. when executing test of pos_mail_receipt something seems blocked.

No output has been received in the last 10m0s...

@PierrickBrun what do you think ?

legalsylvain avatar Jun 23 '22 09:06 legalsylvain

Note to @carmenbianca : I did not explicitly resolve the promise in the code.

  • I am not sure it is needed, since the screen is locked until the mail is sent (or raise an errors)
  • Also after some trials I did not find a way to do it
    • await does not work (I have an error telling me it must be used inside an async function/module/class, although I specifically added async to the function)
    • using .resolve() in the email function does not work either (.resolve() is not a function error)

victor-champonnois avatar Jun 23 '22 09:06 victor-champonnois

@victor-champonnois : not sure that is a time out problem. when executing test of pos_mail_receipt something seems blocked.

No output has been received in the last 10m0s...

@PierrickBrun what do you think ?

@legalsylvain You're right, apparently the problem is related to a call to _wkhtmltopdf in the tests.

victor-champonnois avatar Jun 23 '22 10:06 victor-champonnois

@legalsylvain This is tricky. The timeout happens when the test call send_mail_receipt(), itself calling _run_wkhtmltopdf to convert the receipt to a pdf. _run_wkhtmltopdf run wkhtmltopdf in a subprocess. I tried to run the command via the command line and my conclusion is that it is stuck because of the head:

                <link type="text/css" rel="stylesheet" href="/web/content/572-85cd6e9/web.report_assets_pdf.0.css"/>
                <link type="text/css" rel="stylesheet" href="/web/content/569-a6cdbb7/web.report_assets_common.0.css"/>
                <link type="text/css" rel="stylesheet" href="/web/content/570-a6cdbb7/web.report_assets_common.1.css"/>

When I remove it everything is fine. I think these links cannot be accessed in the test environment. I don't know how to fix this though. It seems impossible to remove the header this high in the call stack. Maybe we need to simply remove this test ?

victor-champonnois avatar Jun 23 '22 15:06 victor-champonnois

I dont know. @ivantodorovich any idea on that topic ?

legalsylvain avatar Jun 23 '22 15:06 legalsylvain

I've never encountered this myself but I see something that bugs me here: https://github.com/OCA/pos/blob/df17842e7cdce02eae825c1133c905dc2ae884f5/.travis.yml#L16

I'm not sure that's the right way of adding wkhtmltopdf dependency, as Odoo requires very specific versions of it.

On the oca repo template we have this: https://github.com/OCA/oca-addons-repo-template/blob/8f81ed76dd5ad0e315fb3b9729e3d7c9eeb4c867/src/%7B%25%20if%20ci%20%3D%3D%20'Travis'%20%25%7D.travis.yml%7B%25%20else%20%25%7D.t2d.yml%7B%25%20endif%20%25%7D.jinja#L67

Sadly the 12.0 branch is not using copier at all, but maybe you could try adding the travis dependency in the same way. Not sure it'll fix the issue, but maybe it's worth trying.

I tried to run the command via the command line and my conclusion is that it is stuck because of the head ... https://github.com/OCA/pos/pull/428#issuecomment-1164544986

Maybe wkhtmltopdf doesn't have access to the odoo server on the test environment? Kind of reminds me of the issues you can get due to misconfiguration and report.url system parameter

ivantodorovich avatar Jun 23 '22 15:06 ivantodorovich

@ivantodorovich Thanks a lot, it works :)

victor-champonnois avatar Jun 24 '22 09:06 victor-champonnois

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). 🤖

OCA-git-bot avatar Jun 24 '22 09:06 OCA-git-bot