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

[ADD] l10n_fr_pos_cert : print hash on the bill, or print a message, if the server is unreachable. #26314

Open
wants to merge 8 commits into
base: 10.0
from

Conversation

Projects
None yet
5 participants
@legalsylvain
Copy link
Contributor

legalsylvain commented Aug 10, 2018

This PR is :

  • based on the refactor of the module proposed to the OCA l10n_fr_certification_pos_offline for the V10 version. (PR)
  • Original Talk about certification with OCA members and Odoo SA members. (Ref 1 ; Ref 2)

Features

It adds the hash on the bill, in the point of sale, if the company is flagged as _is_accounting_unalterable by the module l10n_fr_certification.
When confirming an order, the PoS will wait the creation of the order. if success, the hash will be printed, otherwise, a warning message will be printed.

Todo List @legalsylvain

  • CLA signed.
  • update module version.
  • Adapt the module to not introduce model changes.
  • Apply simplification. (remove undesired features normal_or_block and print_or_block options that prevent to realize a sale if the server is unreachable,)
  • Adapt the code to allow the overload of this module if some people want to have a behaviour more strict, blocking the print of the bill, if the server is unreachable, or to change the text printed on the bill.
  • Test PDF / screen : OK. (iface_print_via_proxy = False)
  • Test printed Bill : (iface_print_via_proxy = True)
  • test with W/O all the odoo pos_ modules installed (pos_restaurant, pos_cache, etc...)
  • Update Pot File. Add French translation.

Related extra works

  • As this feature introduce a non asynchroneous call to create pos order (the Pos will wait the result to print the hash or the warning), this will fail if a timeout occured. for the time being, the call of the function _save_to_server has an hardcoded time out of 7.5 second, that can be not enough if the server is slow. (create an order generate an order and order lines, a picking, + moves and quants). for that purpose, an OCA V10.0 module is available here https://github.com/OCA/pos/tree/10.0/pos_timeout

  • The warning message says that the customer can ask later for a certified bill but in the Odoo, it is not possible to print later a done pos.order in Odoo by default. For that purpose, a GRAP V8.0 module is available here https://github.com/grap/grap-odoo-incubator/tree/8.0/pos_done_order_load. See comments

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

CC : @fgi-odoo, @mart-e, @lap-odoo

Thanks for your review !

@mart-e mart-e requested review from kebeclibre and jorenvo Aug 14, 2018

}
return _t("Because of a network problem," +
" this ticket could not be certified." +
" You can ask later for a certified ticket.");

This comment has been minimized.

@mart-e

mart-e Aug 14, 2018

Contributor

is that really true? can we reprint a ticket with the hash later?

This comment has been minimized.

@legalsylvain

legalsylvain Aug 14, 2018

Author Contributor

In the front office

  • a module exist pos_reprint (in the Odoo core), but AFAIK, it only allows cashier to reprint immediately a receipt, not later. (just the last one)
  • I think it's a missing feature. In many shop, the cashier ask "do you want the bill", the customer say no, and the cashier put the bill in the bin. Finally the customer say yes !
  • For that purpose, I developped a module to do that (to reload done order). See my comment in the main text of the PR : "The warning message says that the customer can ask later for a certified bill but in the Odoo, it is not possible to print later a done pos.order in Odoo by default. For that purpose, a GRAP V8.0 module is available here https://github.com/grap/grap-odoo-incubator/tree/8.0/pos_done_order_load"

in the back office
We had the possibility to print a pdf (qweb) of a pos order (like a sale order, an invoice, etc...) until V9.
Ref. Not available in V10. Do you know why ? If the qweb exist, I can easily add the hash on it.

This comment has been minimized.

@mart-e

mart-e Aug 14, 2018

Contributor

They were removed at 0650047 because they were quite bad. However, I do agree we need to be able to reprint past tickets from the pos interface. we were talking not long ago with @lap-odoo and hopefully, in v13 we can get this.

This comment has been minimized.

@legalsylvain

legalsylvain Aug 14, 2018

Author Contributor

V13 ! OK.

  • So, immediately, I propose to change the text for a more generic one. "Because of a network problem, this ticket could not be certified."
  • When you'll talk again with @lap-odoo, about the reprint feature, please consider that some shops now propose to send the bill by email too. (The qweb report can be interesting for that).
});
}
} else {
// Weird core feature, print_receipt is called regularly

This comment has been minimized.

@mart-e

mart-e Aug 14, 2018

Contributor

I think it is because, each time the printer get connected, it retries to print tickets that may be waiting (but @jorenvo can probably confirm this)

This comment has been minimized.

@legalsylvain

legalsylvain Aug 14, 2018

Author Contributor

Yes, what I don't exactly understand, is why sometimes, print_receipt is call without receipt arg. (receipt=False). but I wrote this comment 6 monthes ago, don't remember exactly the context. I can remove it if you want.

@mart-e
Copy link
Contributor

mart-e left a comment

Add Fr translation. (required ? or do you have an extra system to handle translation ? there is no fr.po file in the current module...)

This module won't be on transifex so if you want, you can add this pot file to your PR and add translation file alongside.

legalsylvain added some commits Aug 14, 2018

[FIX] remove useless ERR_MSG message text in python file.
[UPD] pot file.
[ADD] french translation
[IMP] change the message regarding the legality of certification

@legalsylvain legalsylvain force-pushed the legalsylvain:10.0_l10n_fr_pos_cert__ADD_print_hash branch to 43eca98 Aug 14, 2018

@legalsylvain

This comment has been minimized.

Copy link
Contributor Author

legalsylvain commented Aug 14, 2018

Hi @mart-e. Thanks for your review. I made some changes.

  • removing the part of the text "you can ask later for a certified ticket"
  • remove comment in JS file
  • updating pot file
  • adding french po file
  • I changed also a message here, because the company that should provide the certificate is
    • either the company that developed the software (So odoo SA),
    • either the last IT company that changed significatively the source code. (So in case of Odoo partners that makes huge projects with a lot of customization like CampToCamp, Acsone, etc...), the certification should be given by the integrator in some cases.

French governement Reference question n°34

Si un logiciel est assez ouvert pour permettre à un intégrateur de paramétrer l'inaltérabilité, la
sécurisation, la conservation ou l'archivage, c'est cet intégrateur en tant que dernier intervenant qui est
qualifié d'éditeur (BOI TVA-DECLA-30-10-30, § 310). En tant que tel, c'est à lui de fournir une
attestation ou d'obtenir une certification par un organisme accrédité.

Awaiting other remarks. Did you take a look on the time out possible trouble ? If the server is slow (pos.order creation > 7.5 second) , the warning message will be printed.

kind regards.

@legalsylvain

This comment has been minimized.

Copy link
Contributor Author

legalsylvain commented Aug 20, 2018

Hi @mart-e. Is it OK for you ? Or do you want I change somethings ?
Regards.

@flotho

This comment has been minimized.

Copy link
Contributor

flotho commented Sep 17, 2018

Hi @mart-e , @legalsylvain ,

Please have a look to the comment OCA/l10n-france#126 (comment) and share your thought regarding the performance aspect.
Regards

@mart-e

This comment has been minimized.

Copy link
Contributor

mart-e commented Sep 18, 2018

@legalsylvain the PR looks good on the technical side. I still let @kebeclibre give his opinion (and I think @lap-odoo still wants to test it).

@flotho I am not familiar with the queue process from OCA so do not have an opinion but it is not something we can use in core anyway (could be discussed to integrate this upstream but that is a different topic). Instead I would investigate (e.g. using flamegraph) why it takes so much time in the first place.
Anyway let's stick on the topic of l10n_fr_pos_cert on this pr please.

Promise that will be resolved, when the hash of the saved order is
known
*/
var certification_deferred = null;

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

I would prefer certification_deferred to be a member of this.pos in Models, Devices and PosBaseWidgets

}

return PosModelParent._save_to_server.apply(this, arguments).then(function(server_ids) {
if (server_ids) {

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

wouldn't if (server_ids.length) be enough ?

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

thanks for the simplification !

// Try to get hash of saved orders, if required
var posOrderModel = new DataModel('pos.order');
return posOrderModel.call(
'get_certification_information', [server_ids], false

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

_save_to_server and get_certification_information are @multi methods. I don't get why we fill the hash only for the current order, and not for the potential other orders

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

The hash will be usefull only for the current order.
Scenario 1 (online) : call of _save_to_server is done only for the current validated order. (--> OK)
Scenario 2 (offline) : 10 orders are validated offline. (the bill will be printed each time with the warning message). when the connexion will go back, for the 11th order, (the current) _save_to_order will be called for 11 orders, and the 11th bill will be printed will the hash. (--> OK).

in wich scenario is it usefull to get hash of the past orders (with bill yet printed) ?

if (result.pos_reference.indexOf(current_order.uid) > 0) {
hash = result.l10n_fr_hash;
current_order.set_hash(hash, setting);
}

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

return to break the loop

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

indeed. thanks.

correctly created in the promise 'certification_deferred'
*/
var PosModelParent = models.PosModel.prototype;
models.PosModel = models.PosModel.extend({

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

why not posModel.include ?

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor
  1. For some Odoo JS Model, I can not call super function. (it doesn't work, or I maybe I do wrong some thing).
    for that reason, I write a extend + apply on the ParentModel object.
  2. Odoo itself override the model with that method. See : https://github.com/odoo/odoo/blob/11.0/addons/pos_cache/static/src/js/pos_cache.js#L11

could you explain what I should do ?

kind regards.

This comment has been minimized.

@kebeclibre

kebeclibre Sep 26, 2018

Contributor

@legalsylvain
Woops, my bad.
PosModels is a Backbone class, and I guess the way to extend their behavior is indeed what you do.

On the other hand, PosWidgets like ProxyDevice are regular OdooClass extension, which should support
this._super.apply(this, arguments) instead of prototype.function()

recovered.
*/
var OrderParent = models.Order.prototype;
models.Order = models.Order.extend({

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

why not models.Order.include ?

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

same response as for posModel.

* @param {string} setting: value of the pos_config.l10n_fr_print_hash
* @returns {string}: Certification Text that will be printed on the bill
*/
var prepare_certification_text = function(hash, setting){

This comment has been minimized.

@kebeclibre

kebeclibre Sep 18, 2018

Contributor

I would prefer this method to be a member of models.Order

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

Indeed, but It is not possible because when it is called by the function print_receipt_certification, the printed order is not accessible.
Any workaround ?

@flotho

This comment has been minimized.

Copy link
Contributor

flotho commented Sep 19, 2018

@flotho I am not familiar with the queue process from OCA so do not have an opinion but it is not something we can use in core anyway (could be discussed to integrate this upstream but that is a different topic). Instead I would investigate (e.g. using flamegraph) why it takes so much time in the first place.
Anyway let's stick on the topic of l10n_fr_pos_cert on this pr please.

Hi @mart-e , OCA queue was simply a proposal to make asynchron call of the picking validation during the pos.order create_from_ui process.
Please be advice that there is a HUGE difference between the direct printing and sending the message to the server + hashing (~2sec)+ validating picking (reservation etc... : 3-4 secs more). Our latest tests confirmed that create_picking is the slowing part.
For frequent transactions in the POS, our customers (so yours) won't appreciate this drawback.
What I'm proposing is to split the following method https://github.com/odoo/odoo/blob/10.0/addons/point_of_sale/models/pos_order.py#L535 because IMHO this part is not necessary to be achieved for the certification part. Once it has been split, either working with ir.cron to validate picking (which is part of the core) or split the .js part into 2 parts, one sending and waiting for the financial information and Hash the other validating the picking.

I hope that you would consider that the great work delivered by @legalsylvain won't be appreciated correctly by our customers if the delay to print the receipt change from mostly immediate to 5 to 9 seconds.

Anyway, thanks fro the time spent to read this comment

@legalsylvain
Copy link
Contributor Author

legalsylvain left a comment

Hi I just go back from hollidays. Thanks @kebeclibre for your remarks. I made some changes and asked some questions. Can you take a look ?
kind regards.

correctly created in the promise 'certification_deferred'
*/
var PosModelParent = models.PosModel.prototype;
models.PosModel = models.PosModel.extend({

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor
  1. For some Odoo JS Model, I can not call super function. (it doesn't work, or I maybe I do wrong some thing).
    for that reason, I write a extend + apply on the ParentModel object.
  2. Odoo itself override the model with that method. See : https://github.com/odoo/odoo/blob/11.0/addons/pos_cache/static/src/js/pos_cache.js#L11

could you explain what I should do ?

kind regards.

recovered.
*/
var OrderParent = models.Order.prototype;
models.Order = models.Order.extend({

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

same response as for posModel.

* @param {string} setting: value of the pos_config.l10n_fr_print_hash
* @returns {string}: Certification Text that will be printed on the bill
*/
var prepare_certification_text = function(hash, setting){

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

Indeed, but It is not possible because when it is called by the function print_receipt_certification, the printed order is not accessible.
Any workaround ?

}

return PosModelParent._save_to_server.apply(this, arguments).then(function(server_ids) {
if (server_ids) {

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

thanks for the simplification !

if (result.pos_reference.indexOf(current_order.uid) > 0) {
hash = result.l10n_fr_hash;
current_order.set_hash(hash, setting);
}

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

indeed. thanks.

// Try to get hash of saved orders, if required
var posOrderModel = new DataModel('pos.order');
return posOrderModel.call(
'get_certification_information', [server_ids], false

This comment has been minimized.

@legalsylvain

legalsylvain Sep 25, 2018

Author Contributor

The hash will be usefull only for the current order.
Scenario 1 (online) : call of _save_to_server is done only for the current validated order. (--> OK)
Scenario 2 (offline) : 10 orders are validated offline. (the bill will be printed each time with the warning message). when the connexion will go back, for the 11th order, (the current) _save_to_order will be called for 11 orders, and the 11th bill will be printed will the hash. (--> OK).

in wich scenario is it usefull to get hash of the past orders (with bill yet printed) ?

@kebeclibre

This comment has been minimized.

Copy link
Contributor

kebeclibre commented Sep 26, 2018

@legalsylvain
I allowed myself to test and do some stuff, tell me what you think

From what I tested:

  1. You set the hash to false prior to do anything, it's a good thing

  2. You only modify the hash when there is one (the server responded)

  3. the receipt takes the order to render, which with 1 & 2 shall contain the right values
    => there is no need to modify the rendered receipt ex-post

  4. print_receipt is at the end of the stack, so only it can nullify the deferred

  5. since everything depends on receiptWidget.show (because it renders the receipts there) this is the only function that should really wait for the deferred.

  6. from 5: print_receipt can check the deferred as well, but it can have the information whether the order has a hash on its own (like when clicking multiple times on "print receipt")

There was a small bug:
calling set() on a posModel, makes it save to db, so without my override the paid order with hash would have been in localdata.unpaid_orders (there was a misbehavior when hitting F5 subsequently)

And there is that weird stuff where odoo Class should support _super, but do not for some reason :D

@legalsylvain

This comment has been minimized.

Copy link
Contributor Author

legalsylvain commented Sep 26, 2018

Hi @kebeclibre. Thanks for the changes. looks nice ! I don't have the time to make a deep check. I'll be glad to take a time with you the next week during the odoo experience, to finish this work. Do you think it's possible ?
regards.

}, function error() {
self.show_certification();
this._handleCertification().always(function () {
ReceiptScreenWidgetShowParent.apply(self, selfArgs);

This comment has been minimized.

@kebeclibre

kebeclibre Sep 27, 2018

Contributor

Admittedly, there might be too soon to wait for the deferred (it cold have been done in render_receipt), but it has the advantage of:

  • covering both the rendering of the ticket for display onto the screen
  • calling the device.print() method with the deferred resolved
  • preventing the user to compulsively manually click on "print receipt"
@flotho

This comment has been minimized.

Copy link
Contributor

flotho commented Oct 5, 2018

Hi @legalsylvain , Any updates from OdooDays on this one ?

@legalsylvain

This comment has been minimized.

Copy link
Contributor Author

legalsylvain commented Oct 10, 2018

Hi @flotho. I talked a while with @kebeclibre during the Odoo days.
The next step for this work is to be tested by @lap-odoo, and then it could be merged, if validated.

regards.

@legalsylvain

This comment has been minimized.

Copy link
Contributor Author

legalsylvain commented Oct 18, 2018

Hi @lap-odoo, any news on that PR.
kind regards.

@lap-odoo

This comment has been minimized.

Copy link
Contributor

lap-odoo commented Oct 19, 2018

@legalsylvain I've just tested it this morning and noticed some issues (with the reprint of a ticket for example). @kebeclibre and @mart-e will have a look at it. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment