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

11.0 mig sale team operating unit #100

Merged
merged 6 commits into from
Mar 20, 2018
Merged

11.0 mig sale team operating unit #100

merged 6 commits into from
Mar 20, 2018

Conversation

gdgellatly
Copy link
Contributor

@gdgellatly gdgellatly commented Nov 3, 2017

Depends on

Just needed a view fix as more_info page is now gone. Put with company as per v10.

@gdgellatly gdgellatly mentioned this pull request Nov 3, 2017
2 tasks
@pedrobaeza pedrobaeza mentioned this pull request Nov 3, 2017
11 tasks
@pedrobaeza pedrobaeza added this to the 11.0 milestone Nov 3, 2017
@AaronHForgeFlow
Copy link
Contributor

Why are you including the operating_unit module fields here? #99 means to do that, doesn't' it?

@gdgellatly
Copy link
Contributor Author

@aheficent That is correct, this PR extends the last one, they include the same commits so they won't duplicate, they are all mergeable, but need to merge in order. I don't really know how to explain it, but its right. Maybe @pedrobaeza can explain better.

@gdgellatly
Copy link
Contributor Author

@aheficent To review you are best to just take the last commit, 10f25f4 that is just the changes from v10 to v11 for sale_team

@AaronHForgeFlow
Copy link
Contributor

It's ok then. I'd preferred to do that using the oca_dependencies.txt file because now I get some merge conflicts when merging the two branches.

@gdgellatly
Copy link
Contributor Author

Hmmm, I think I made a mistake then, I'll look on Monday, but I did a merge commit when I probably shouldn't have. Was my first time having to follow this process for multiple dependent modules.

@AaronHForgeFlow
Copy link
Contributor

np @gdgellatly as runbot is not working for v11 and I have to test in local anyway it's ok as it is

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

IMHO I should not be able to add user to a sales team with an operating unit when the user it's not assigned to that operating unit.

@pedrobaeza
Copy link
Member

About including commits of dependent modules: this is another technique not specified in the migration guide. What this does is to allow to test the module in this PR without waiting the dependency to be merged, but has these disadvantages:

  • Confuse the commit list
  • If the dependency PR evolves, this PR doesn't reflect it unless you make a git operation. In this case, the best is to rebase over dependency branch, instead of merging, or the history will be totally messed.
  • When the other PR is merged, and more if a squashing operation is performed before merging, you have to make an interactive rebasing on this PR, removing manually commits from the other PR, or you will get a conflict.

@gdgellatly
Copy link
Contributor Author

@aheficent As I say, these are straight ports. So the functionality has not changed. I can kind of see both cases, but if people agree I'll add the domain. It's not hard.

@gdgellatly
Copy link
Contributor Author

I'll probably just create a new branch and cherry pick as we merge up and then force push as we merge each PR. Thats why I'm only doing a few modules at a time in dependent order. In the meantime at least we can get reviews sorted.

@AaronHForgeFlow
Copy link
Contributor

@gdgellatly It's ok to me to keep the functionality as it was in v10. Better to create a PR with the fix in v10 and forward port it to v11 after.

@simahawk
Copy link

@gdgellatly can you drop #99 commits and use oca_dependencies.txt? I know it makes your life easier in the short term but it gets harder for you and reviewers in the long one ;)

@simahawk
Copy link

@gdgellatly #99 got merged: you can rebase 😉

Copy link

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

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

Just a small change needed.

<!-- Copyright 2016-17 Eficent Business and IT Consulting Services S.L.
Copyright 2017-TODAY Serpent Consulting Services Pvt. Ltd.
License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl-3.0) -->
<odoo noupdate="0">

Choose a reason for hiding this comment

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

noupdate should be 1

@gdgellatly
Copy link
Contributor Author

@simahawk it was already rebased and updated to work with changes in #99


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/213/10.0

Choose a reason for hiding this comment

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

/11.0

@@ -0,0 +1,5 @@
# © 2015-2017 Eficent

Choose a reason for hiding this comment

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

Copyright

"author": "Eficent, "
"Serpent Consulting Services Pvt. Ltd.,"
"Odoo Community Association (OCA)",
"website": "http://www.eficent.com",

Choose a reason for hiding this comment

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


def setUp(self):
super(TestSaleTeamOperatingUnit, self).setUp()
self.res_users_model = self.env['res.users']

Choose a reason for hiding this comment

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

.with_context(tracking_disable=True, no_reset_password=True) to avoid mail.thread overhead and avoid email creation overhead (and they are actually sent, BTW :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simahawk Most of those issues were long fixed as part of rebase. Not sure how you even found them given the code was force pushed and overwritten.

- Remove encodings
- Bump Version
- Fix form view to place under company_id
- Test
- Fix security rules
@JordiBForgeFlow JordiBForgeFlow merged commit 72e1d9a into OCA:11.0 Mar 20, 2018
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.

None yet

9 participants