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

feat: Grant commission on certain items only #27467

Merged
merged 6 commits into from
Nov 30, 2021

Conversation

barredterra
Copy link
Collaborator

@barredterra barredterra commented Sep 13, 2021

Close #27276

  • "Amount Eligible for Commission" is calculated as the sum of net amounts of line items where "Grant Commission" is active.
  • Total Commission = Amount Eligible for Commission * Commission Rate
Bildschirmaufnahme.2021-09-13.um.23.05.18.mov

Docs:

Thanks to @aisenyi and @sagarvora for their help with this feature.

@barredterra barredterra changed the title Grant commission feat: Grant commission on certain items only Sep 13, 2021
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #27467 (a9bd71e) into develop (d0f4f03) will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@             Coverage Diff             @@
##           develop   #27467      +/-   ##
===========================================
+ Coverage    55.22%   55.23%   +0.01%     
===========================================
  Files         1121     1121              
  Lines        66746    66748       +2     
===========================================
+ Hits         36859    36869      +10     
+ Misses       29887    29879       -8     
Impacted Files Coverage Δ
erpnext/stock/get_item_details.py 79.60% <ø> (+0.15%) ⬆️
erpnext/controllers/selling_controller.py 78.12% <85.71%> (-0.18%) ⬇️
erpnext/controllers/accounts_controller.py 83.98% <100.00%> (ø)
...ctype/woocommerce_settings/woocommerce_settings.py 80.00% <0.00%> (-4.00%) ⬇️
...e/asset_value_adjustment/asset_value_adjustment.py 86.04% <0.00%> (-3.49%) ⬇️
erpnext/education/doctype/student/student.py 73.68% <0.00%> (-3.16%) ⬇️
...ion/doctype/course_enrollment/course_enrollment.py 44.00% <0.00%> (-2.00%) ⬇️
...ctype/accounting_dimension/accounting_dimension.py 64.06% <0.00%> (-1.57%) ⬇️
...e/period_closing_voucher/period_closing_voucher.py 88.23% <0.00%> (-1.48%) ⬇️
erpnext/portal/utils.py 28.98% <0.00%> (-1.45%) ⬇️
... and 16 more

@mujeerhashmi
Copy link
Contributor

Oh, I totally missed out on this feature. It's great. One suggestion, the commission rate % may not be the same for all the items in a sales invoice. So defining the commission rate % in item master would be more feature-rich. Then calculate the commission per line item & totaling them would be more extensive.

@barredterra
Copy link
Collaborator Author

@mujeerhashmi currently the commission is defined per Sales Partner. So to make your suggestion work we would need a mapping between Item and Sales Partner. This makes things much more complicated, so I would suggest waiting until someone really has this requirement.

@barredterra
Copy link
Collaborator Author

@marination any other changes required?

@sagarvora sagarvora added dont-merge needs-tests This PR needs automated unit-tests. labels Oct 4, 2021
@sagarvora sagarvora self-assigned this Oct 4, 2021
@sagarvora sagarvora marked this pull request as draft October 4, 2021 13:44
@sagarvora sagarvora removed dont-merge needs-tests This PR needs automated unit-tests. labels Oct 4, 2021
@sagarvora sagarvora assigned sagarvora and unassigned sagarvora Oct 4, 2021
@sagarvora sagarvora force-pushed the grant-commission branch 2 times, most recently from e0f9449 to 879895e Compare October 6, 2021 11:16
@sagarvora sagarvora marked this pull request as ready for review October 6, 2021 11:18
@sagarvora
Copy link
Collaborator

sagarvora commented Oct 6, 2021

Additional changes made:

  • Make Grant Commission in Transaction Item Read Only
  • Rename Commission Base to Amount Eligible for Commission
  • Calculate Amount Eligible for Commission in backend as well
  • Ignore Quotation, but include POS Invoice when calculating commission and contribution in the backend
  • Add Test Case
  • Semantic / Code Quality fixes

@barredterra Please update documentation if needed.

@stale stale bot added the inactive label Nov 12, 2021
@hrwX
Copy link
Contributor

hrwX commented Nov 26, 2021

@barredterra This looks good, works as expected 👌

@sagarvora
Copy link
Collaborator

@marination @deepeshgarg007 @nextchamp-saqib
It's been almost 2 months since this PR was last changed. Is it safe to assume that no further changes are required?

@frappe frappe deleted a comment from stale bot Nov 29, 2021
@nextchamp-saqib
Copy link
Member

Looks good to me!

@sagarvora sagarvora added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Nov 30, 2021
@sagarvora sagarvora merged commit e10ab16 into frappe:develop Nov 30, 2021
mergify bot pushed a commit that referenced this pull request Nov 30, 2021
Co-authored-by: Sagar Vora <sagar@resilient.tech>
(cherry picked from commit e10ab16)

# Conflicts:
#	erpnext/selling/sales_common.js
#	erpnext/stock/doctype/item/item.json
@mergify
Copy link
Contributor

mergify bot commented Nov 30, 2021

backport version-13-hotfix

✅ Backports have been created

@@ -1020,6 +1021,12 @@
"fieldname": "website_image_alt",
"fieldtype": "Data",
"label": "Image Description"
},
{
"default": "1",
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be enabled by default right? @sagarvora @barredterra

Copy link
Collaborator

@sagarvora sagarvora Dec 29, 2021

Choose a reason for hiding this comment

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

Previously, this was applicable for all items. This keeps it backwards compatible. Alternative is patch with default as 0. But I think that there are generally lesser number of items where commission isn't granted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you think it is getting undue attention because the checkbox defaults to 1, we can change the field to be Don't Grant Commission instead.

conncampbell pushed a commit to conncampbell/erpnext that referenced this pull request Jan 9, 2022
Co-authored-by: Sagar Vora <sagar@resilient.tech>
conncampbell pushed a commit to conncampbell/erpnext that referenced this pull request Jan 9, 2022
Co-authored-by: Sagar Vora <sagar@resilient.tech>
@bhavesh95863
Copy link
Contributor

bhavesh95863 commented Jan 10, 2022

Hey @barredterra ,
Why do we not have a patch to update the old document item table? I have updated one of my server and this feature blocks commission calculation.
I show there is a grant_commission checkbox in the item table but that value is 0 and because of that amount_eligible_for_commission is 0.

fproldan added a commit to fproldan/erpnext that referenced this pull request Jan 2, 2023
fproldan added a commit to fproldan/erpnext that referenced this pull request Jan 2, 2023
ValentinaPruvost pushed a commit to fproldan/erpnext that referenced this pull request Jan 3, 2023
* feat: Grant commission on certain items only (backport frappe#27467)

* feat: Grant commission on certain items only (backport frappe#27467)

* fix

* fix
fproldan added a commit to fproldan/erpnext that referenced this pull request Jan 4, 2023
* feat: Grant commission on certain items only (backport frappe#27467)

* feat: Grant commission on certain items only (backport frappe#27467)

* fix

* fix
ValentinaPruvost pushed a commit to fproldan/erpnext that referenced this pull request Apr 17, 2023
* feat: Grant commission on certain items only (backport frappe#27467)

* feat: Grant commission on certain items only (backport frappe#27467)

* fix

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squash Meant to tell reviewers that this PR should be squashed into a single commit while merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commission on certain Items only
7 participants