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.

Sign up for GitHub

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

Jump to bottom

ADD sale_commission_geo_assign #183

Merged
merged 1 commit into from
Nov 12, 2018
Merged

ADD sale_commission_geo_assign #183

merged 1 commit into from
Nov 12, 2018

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Oct 30, 2018

No description provided.

@eLBati eLBati force-pushed the 10.0-sale_commission_geo_assign branch from e073cc1 to 6be2139 Compare October 30, 2018 13:19
@eLBati
Copy link
Member Author

eLBati commented Oct 30, 2018

@pedrobaeza This explains a bit why I made #181

@eLBati eLBati added this to the 10.0 milestone Oct 30, 2018
tafaRU
tafaRU approved these changes Oct 30, 2018
View reviewed changes
Copy link
Member

@tafaRU tafaRU left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some nitpicking but none blocking.
EDIT: Tested locally as runbot instance seems do not work properly

sale_commission_geo_assign/__manifest__.py Outdated
"views/res_partner_view.xml",
"wizard/wizard_geo_assign_partner_view.xml",
],
"demo": [
Copy link
Member

Choose a reason for hiding this comment

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

please remove empty key

Copy link
Member Author

Choose a reason for hiding this comment

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

done

sale_commission_geo_assign/tests/test_sale_commission_geo_assign.py Outdated
self.assertFalse(c1.agents)

agent2 = self.partner_model.create({
'name': 'agent1',
Copy link
Member

Choose a reason for hiding this comment

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

maybe agent2?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

sale_commission_geo_assign/tests/test_sale_commission_geo_assign.py
'zip_from': '16100',
'zip_to': '17100',
})
wizard.geo_assign_partner()
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't raise an exception considering that c2 has already agents? 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

@tafaRU no, because wizard.check_existing_agents = False

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I missed it! Thanks for the explanation.

@eLBati eLBati force-pushed the 10.0-sale_commission_geo_assign branch from 6be2139 to bd309a1 Compare October 30, 2018 14:32
SimoRubi
SimoRubi suggested changes Oct 30, 2018
View reviewed changes
Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

sale_commission_geo_assign/__manifest__.py Outdated
"views/res_partner_view.xml",
"wizard/wizard_geo_assign_partner_view.xml",
],
"demo": [
Copy link
Member

Choose a reason for hiding this comment

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

remove

sale_commission_geo_assign/models/res_partner.py Outdated
class Partner(models.Model):
_inherit = 'res.partner'

country_ids = fields.Many2many(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this field name is a bit too generic, something like agent_country_ids or similar would be better I think.
Same for other fields

sale_commission_geo_assign/wizard/wizard_geo_assign_partner.py Outdated
raise UserError(_(
"Partner %s already has agents. You should remove them"
" or deselect 'Check existing agents'"
) % partner.name)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe display_name would be better

sale_commission_geo_assign/models/res_partner.py Outdated
}}

@api.multi
def verify_agent(self, partner):
Copy link
Member

Choose a reason for hiding this comment

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

Please add a little documentation to make it clear what this method does: Check if it is ok to assign this agent to the partner or similar

sale_commission_geo_assign/README.rst

Sales > Commissions Management > Agents

For every agent, you can set Countries, States or ZIP range
Copy link
Member

Choose a reason for hiding this comment

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

Can't find the fields in runbot:
image

Copy link
Member

Choose a reason for hiding this comment

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

Indeed! I made my own tests locally as It's written in my review comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because of

commission/sale_commission_areamanager/views/res_partner_view.xml

Line 9 in 5dd3063

<page name="agent_information" position="replace">
...
I am going to fix

Copy link
Member Author

Choose a reason for hiding this comment

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

@SimoRubi I made the other changes meanwhile

Copy link
Member Author

Choose a reason for hiding this comment

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

@tafaRU @SimoRubi here it is #184

@eLBati eLBati force-pushed the 10.0-sale_commission_geo_assign branch from bd309a1 to adc4ddd Compare October 30, 2018 14:54
@pedrobaeza
Copy link
Member

Then add here that filter, not in the general one 😉

Any way, you should put only one of them (the most relevant one) IMO.

@eLBati eLBati force-pushed the 10.0-sale_commission_geo_assign branch 2 times, most recently from 96c2d84 to f67398b Compare October 30, 2018 15:10
@eLBati
Copy link
Member Author

eLBati commented Oct 30, 2018

@pedrobaeza right, done

This was referenced Oct 30, 2018
Closed
Closed
@eLBati eLBati force-pushed the 10.0-sale_commission_geo_assign branch from f67398b to fa776aa Compare November 5, 2018 08:18
@eLBati
Copy link
Member Author

eLBati commented Nov 5, 2018

Merged #184 and rebased this

SimoRubi
SimoRubi approved these changes Nov 12, 2018
View reviewed changes
Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

code & runbot review, ok for me

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

@tafaRU tafaRU merged commit b4c5991 into OCA:10.0 Nov 12, 2018
iwkse pushed a commit to iwkse/commission that referenced this pull request Mar 14, 2022
@eLBati @iwkse
ADD sale_commission_geo_assign (OCA#183)
c8af749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tafaRU tafaRU tafaRU approved these changes

@SimoRubi SimoRubi SimoRubi approved these changes

Assignees
No one assigned
Labels
approved needs review ready to merge
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@eLBati @pedrobaeza @OCA-git-bot @tafaRU @SimoRubi