Skip to content
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

[14.0][IMP]sale_commission: save settlement wizard date in settlement #578

Closed
wants to merge 1 commit into from

Conversation

aleuffre
Copy link

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @pedrobaeza,
some modules you are maintaining are being modified, check this out!

@aleuffre aleuffre marked this pull request as draft November 13, 2024 16:39
@aleuffre aleuffre marked this pull request as ready for review November 13, 2024 16:48
@aleuffre aleuffre force-pushed the 14.0-settlement-wizard-dates branch from e1896a7 to 3b1ef1f Compare November 13, 2024 16:48
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Functional ok!

@pedrobaeza this IMP follows this use case:

Settlements currently displays "Date from" and "Date to" which are taken from invoice date

With more complex setups (eg: commission types based on payment, settlement line created proportionally to payment received) these dates are not very relevant, as a settlement can be created in November 2024 for a payment received on an invoice from June 2024

These fields allow user to know the dates inputted in wizard to create settlement, eg:

"this settlement is for invoices issued from 1st to 30th of June 2024, based on invoices with date up to November 1st 2024 and payments received up to November 20th 2024"

Copy link
Contributor

@renda-dev renda-dev left a comment

Choose a reason for hiding this comment

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

Code review, LGTM

@francesco-ooops
Copy link
Contributor

@pedrobaeza is this change ok?

@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza added this to the 14.0 milestone Nov 18, 2024
@pedrobaeza
Copy link
Member

I don't really get this need. The creation date already allows to differentiate when the settlement is created, or switch to this approach: #519

@francesco-ooops
Copy link
Contributor

@pedrobaeza in this specific case, I need to recreate all settlements at once, but I still want to be able to group them by invoice and payment date. Creation date is not a relevant value as all settlements would have current date.

From what I understand, #519 is the opposite of what I'm looking for? It's hard to say as there's no rationale for the change or functional instructions.

Anyway, it just makes sense to me that if some values can be selected in the wizard, they can then be saved in the record itself.

Furthermore, we implemented the improvement as not to change anything in current flow, so I fail to see the reason to reject this PR.

@pedrobaeza
Copy link
Member

Then for your specific case, you should create an extra module on top doing that, instead of overloading the base module with more and more options and fields.

@aleuffre aleuffre closed this Nov 19, 2024
@aleuffre aleuffre deleted the 14.0-settlement-wizard-dates branch November 19, 2024 11:35
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.

5 participants