[17.0][ADD] base_import_pdf_by_template_account: New module

[victoralmau]

New module

Locked by:

• pdf_helper: [17.0][FIX] pdf_helper: Odoo compatibility #1057

Please @pilarvargas-tecnativa can you review it?

@Tecnativa TT50003

[victoralmau]

Ping @pedrobaeza What do you think about this approach?

[victoralmau]

It's ready to review.

[pilarvargas-tecnativa]

Remove WIP tag from PR title

[pilarvargas-tecnativa]

It is not necessary?

[victoralmau]

Yes, it is necessary but it is commented because it is not possible to do that directly. If you know the way to do it, it will be welcome! (this is currently set correctly in tests).

[pedrobaeza]

Better to not use demo data on tests, as already stated in other PRs.

[victoralmau]

So in all tests we create everything related to a template again? I do not agree that this is useful in the OCA modules (I am not sure it is an “official” guideline either.).

[pedrobaeza]

You have to test the specific gestures of this module, not an entire template, and yes, what's needed for that tests should be in the test itself.

[victoralmau]

Sorry, but how is the _import_base_import_pdf_by_template() method supposed to be tested without a full template (that template already existed in test_base_import_pdf_by_template so that user installing the module can test it). The template is also useful for testing analytic distributions.

As a personal reflection, i think that all this "bureaucracy" is not good and will only demotivate some contributors.

[pedrobaeza]

This is not bureaucracy. It's doing tests the best way. I already stated the reasons to not use demo data in other PR. Do you remember?

I'm not saying here to not use templates. What I'm saying is that you don't need a whole template, just the minimal data to test the features. And all of that definitions inside the test class, not as demo data.

[victoralmau]

This is not bureaucracy. It's doing tests the best way. I already stated the reasons to not use demo data in other PR. Do you remember?

Yes, and I explained then that I disagreed on the arguments. I explained earlier that you moved the demo data from test_base_import_pdf_by_template (and the tests) to base_import_pdf_by_template_account but never mind, I'll make the change so you don't block the PR. I'll get on it.

[pedrobaeza]

Yes, and I explained then that I disagreed on the arguments

You said that you don't agree, and then I'll explain the reasons, but you didn't counter-argument my explanations to say why it's better to use demo data. Odoo itself has moved to be demo-data independent because of these reasons.

[victoralmau]

It was not worth counter-argument, just like now, it is simply better to make the changes you suggest and that's it.

[pedrobaeza]

OK, we have debated about this internally. Just for others reading this, there's a guideline about this:

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#616avoid-relying-on-demo-data

Let's change it for avoiding problems in the future.

[victoralmau]

Extra commit added (and many lines) to define data in tests (without using demo data). Do you want that change also made to base_import_pdf_by_template and test_base_import_pdf_by_template?

[pedrobaeza]

Do you really need the supplierinfo for all the products?
Do you really need so many products?

When I talked about doing the minimal template in tests, I was referring to that, to have the minimal test unit, not a PDF template with lots of lines that are the same (but with another product).

I don't want to bother you more in this one, but it's for you to take that into consideration. Always design the tests with the minimum use case, not a one with a lot of things repeated, but that are not adding real value to the test, just boilerplate creating more data, but testing the same feature.

[victoralmau]

Do you really need the supplierinfo for all the products? Do you really need so many products?

When I talked about doing the minimal template in tests, I was referring to that, to have the minimal test unit, not a PDF template with lots of lines that are the same (but with another product).

This is importing an existing .pdf file created specifically for that template (it was created some time ago), you can create another one specifically for the tests, but it would not make sense (or at least not to me), precisely for this reason I did not see sense to make this change.

[pedrobaeza]

Well, even if using this not so ideal template, I think you can create just one product with full data, and another "fake one" that you put as fallback in case the product is not found for the rest of the lines. Or is there any exception in that case?

[victoralmau]

The test was intended to validate a multi-line use case with different quantities, prices, etc, but if you want we can change it... (I don't understand that these comments appear on unrelated things, i.e. the test already existed and was validated).