Skip to content

[17.0][IMP] sale_quotation_number: allow customizable quotation sequence#3310

Merged
OCA-git-bot merged 1 commit intoOCA:17.0from
c4a8-odoo:17.0-port-sale_quotation_number-sequence
Nov 18, 2024
Merged

[17.0][IMP] sale_quotation_number: allow customizable quotation sequence#3310
OCA-git-bot merged 1 commit intoOCA:17.0from
c4a8-odoo:17.0-port-sale_quotation_number-sequence

Conversation

@MohamedOsman7
Copy link
Copy Markdown
Contributor

@MohamedOsman7 MohamedOsman7 commented Sep 12, 2024

Enable users to customize the sale.quotation sequence prefix, instead of it being restricted to the default 'SQ'.

Solution from version 14: #2126

(PR 16: #3309 )
(PR 15: #3311 )

(Follow-up PR: #3213)

Copy link
Copy Markdown
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

LGTM code + tested

@rousseldenis
Copy link
Copy Markdown
Contributor

@MohamedOsman7 Ok, next time, the better is to cheery-pick the original fix and then do the modification (the search before the loop) to keep original author. Thanks

@CRogos
Copy link
Copy Markdown
Contributor

CRogos commented Oct 9, 2024

@rousseldenis could you add a review and merge?

@CRogos
Copy link
Copy Markdown
Contributor

CRogos commented Nov 16, 2024

@pedrobaeza could you also merge this?

v17 of: #3309

@pedrobaeza
Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-3310-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 78bdcab into OCA:17.0 Nov 18, 2024
@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

@manuelregidor
Copy link
Copy Markdown
Contributor

@MohamedOsman7 @CRogos @pedrobaeza @rousseldenis @ValentinVinagre @HaraldPanten

This last improvement applied to versions from 14 up to 17 is not working properly.

First of all, there was an improvement already merged in v15 that solved the issues pointed out here. The PR was https://github.com/OCA/sale-workflow/pull/2719/files and there was a fix aftewards: https://github.com/OCA/sale-workflow/pull/2752/files. Is there any reason why these commits were not cherry-picked and another approach was developed instead?

Second, we think the approach previously available in v15 is more convenient, for two reasons basically:

  1. The approach in this PR does not work if the quotation's sequence uses a formula in its prefix. For example, if the sequences's prefix is "Q/%(year)s/" the following line will not work as expected:
    if sequence and self.name[: len(sequence.prefix)] != sequence.prefix:
    As the quotation number will contain the year itself (and not the string "%(year)"), the sequence will be not changed to the sale order sequence when the quotation gets confirmed.

  2. The approach in this PR only considers the sequences with codes "sale.quotation" and "sale.order", whereas the approach previsouly available in v15 uses methods get_quotation_seq and get_sale_order_seq to get the sequence to be applied. Those methods can easily be inherited so other sequences can be used depending on different requirements (for example, in module sale_order_type_quotation_numer https://github.com/OCA/sale-workflow/tree/15.0/sale_order_type_quotation_number).

Some of our clients currently use this module in v15 and are having issues as they use a formula in the quotation sequence prefix. We should find a way to proceed to get this sorted, as the old v15 of the module with an improvement, which worked fine, has stopped working and the new version of the module in the rest of versions will cause problems.

Thank you.

@HaraldPanten
Copy link
Copy Markdown
Contributor

IMO:

  • I think we should revert this last commit (At least in V15). It has been done in a stable version and it's not needed there.
  • Then we should discuss about the best approach for V16 and V17.

WDYT?

@HaraldPanten
Copy link
Copy Markdown
Contributor

HaraldPanten commented Nov 20, 2024

Solving the issue in V15 before discussing the best solution.

#3416

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.

7 participants