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

Handle more complex commission structures #27

Closed

Conversation

veloutin
Copy link
Member

  • Support multiple commissions per agent
  • Support commissions based on product margins
  • Handle period-based commissions

- Support multiple commissions per agent
- Support commissions based on product margins
- Handle period-based commissions
@veloutin
Copy link
Member Author

@pedrobaeza You want to take a look?

@@ -22,7 +22,7 @@
##############################################################################
{
'name': 'Sales commissions',
'version': '2.0',
'version': '2.1',
Copy link
Member

Choose a reason for hiding this comment

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

@pedrobaeza
Copy link
Member

Hi, @veloutin, thanks for the effort of making such changes. I have started reviewing the PR, but I find a lot of difficulties to get the review done because:

  • There are a lot of unneeded renamings. Maybe some field's names are not the best (but they're appropiated anyways), but handling these renamings in migrations scripts is a complex thing, so we shouldn't take care of this in a middle of a version.
  • This PR covers a lot of functionality (scope, multi-agents per partner, etc), and it's very difficult to review. I recommend you to add each of the new features one by one in a PR, so people can review them easily and helps each PR to be merge. If not, I'm afraid this PR will be held here for too many months.
  • I don't agree with some coding decisions like replacing stored tables with some views that can hit performance.

So I ask you to rework on the changes and start with these smaller PRs.

Thanks.

@max3903 max3903 added this to the 8.0 milestone Aug 17, 2015
@max3903 max3903 mentioned this pull request Aug 17, 2015
- Handle invoice cancelling and refunding
- Hide commissions from invoice and sale order form view
- Correctly update to new commission rules when duplicating invoices
@veloutin
Copy link
Member Author

With the latest changes, the use case seems quite different from the original module:

  1. Commissions can be set up by periods, on sales and margins
  2. Each agent can have many different commissions
  3. Commissions are set automatically and not manually
  4. Commission lines can belong to a single settlement
  5. When cancelling an invoice, settled lines are reverted.

I am wondering, is there any way to reconcile the original behavior with this one, or would this be better suited as an alternate module?

@pedrobaeza
Copy link
Member

It can be reconciled without problem with original behaviour. We just need to provide a migration path, in combination with good default values and something that can be reviewable, so please split the changes and make smaller PRs.

@max3903
Copy link
Member

max3903 commented Aug 19, 2015

@pedrobaeza What would be the scope of each PR ?

@pedrobaeza
Copy link
Member

A good scope can be each of the points that @veloutin has pointed.

@max3903
Copy link
Member

max3903 commented Aug 19, 2015

@pedrobaeza The points listed by @veloutin are the steps of the process.

@pedrobaeza
Copy link
Member

Each point can be easily a feature to be developped isolated. I insist to easy the review.

@veloutin
Copy link
Member Author

@pedrobaeza I will try to split it as such and also try to avoid making several PRs with just as big changes, as it wouldn't help the review effort.

@pedrobaeza
Copy link
Member

Thank you very much. I'll review as soon you make the PRs.

@max3903
Copy link
Member

max3903 commented Oct 31, 2015

superseeded by #38

@max3903 max3903 closed this Oct 31, 2015
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.

3 participants