OCA / sale-reporting

GNU Affero General Public License v3.0
50 stars 193 forks source link

[14.0][ADD] sale_order_terms_document #199

Closed remi-filament closed 1 year ago

remi-filament commented 1 year ago

Add a module allowing to add Terms and Conditions in PDF after the generated sale order PDF. Also, this module adds a link to T&C PDF in portal view when signing document

pedrobaeza commented 1 year ago

Please don't add plurals in module names. Anyway, there's already sale_comment_template for this.

remi-filament commented 1 year ago

Hi @pedrobaeza, Thanks for looking into it !

About plural, here I refer to Terms & Conditions, which is why I used plural, not sure if it still makes sense to call it term in this context ?

Also, from my point of view, use case is different from sale_comment_template one. For the following reasons :

  1. Here it is proposed to directly load a PDF file which will be added after sale order (this PDF with Terms and Conditions you probably already have if you are an existing sales company and you do not want to rewrite it fully in Odoo HTML field)
  2. Terms and Conditions make no sense on invoices and therefore should not be present on invoice
  3. You need to make these terms and conditions visible / accessible on portal view so that when customer signs sale order, he acknowledges T&C (this modules allows for this, when sale_comment_template does not)

With the above explanation, do you still think this module is useless (since already covered in another module) ? Best Regards,

pedrobaeza commented 1 year ago

I think you can integrate the PDF possibility on sale_comment_template (well, more on base_comment_template), and also a check about propagating or not to invoice.

remi-filament commented 1 year ago

Yes probably, though it adds a lot of complexity for customers when most of them do not need it...

pedrobaeza commented 1 year ago

I don't think there's too much burden to add this in the other module, and it's very related, but it's up to you to do it. If you continue this way, then the module should be handled as something more generic, as this isn't only for sales orders. It should in OCA/reporting-engine as well. The name may be report_extra_pdf_content or similar, and includes a conditional expression to be safe-evaled to include it or not.

njeudy commented 1 year ago

Pour moi c'est ok d'ajouter ce module tel quel:

  1. parce que ca répond à un besoin client (plusieurs client meme)
  2. parce qu'il fonctionne

For me it's ok to add this module as is:

  1. because it meets a customer need (several customers even)
  2. because it works

I think that we can make this module general if the need is expressed by several people and that it will not pose any problem thereafter?

@pedrobaeza , I would like to understand what are the guidelines or fears that make you want to make it more generic ?

A little module that allows you to add the T&C on the quotes, sounds good to me :)

will review this soon thanks @remi-filament

pedrobaeza commented 1 year ago

My goal is to avoid a user fragmentation and several things for the same goal but different implementations that bloats an instance. It's legit that you have some terms that are written, and another that includes a PDF attached, but I'm not enforcing anything, I'm just giving my opinion. And it's a pity that the feature of having extra PDF content to any report is limited in visibility and applicability in this repo.

It's also very counter-intuitive that for making this module works, you have to go to the report definition and enabling an option that can be enabled in other models reports.

remi-filament commented 1 year ago

I agree with @pedrobaeza that having to configure the boolean in ir.actions.report is confusing. I just removed it from the code, so that adding terms and conditions PDF will only happen on sale.action_report_saleorder. Also, I agree that it could be made more general elsewhere, but we do not have another use case where it makes sense elsewhere and therefore I prefer to keep things simple to answer the very common need from our customers to be able to add terms and conditions on their sale orders without having to configure anything else or add many dependencies that add complexity on my instances.

github-actions[bot] commented 1 year ago

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.