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 partner contact in several companies #507

Merged
merged 21 commits into from
Dec 12, 2017

Conversation

njeudy
Copy link

@njeudy njeudy commented Oct 24, 2017

  • Migrate partner_contact_in_several_companies
  • Migrate partner_contact_personal_information_page

In the same PR because dependency

@njeudy njeudy mentioned this pull request Oct 24, 2017
53 tasks
@njeudy
Copy link
Author

njeudy commented Oct 24, 2017

I think a need to squash :)

But don't know how ?

@njeudy
Copy link
Author

njeudy commented Oct 24, 2017

@pedrobaeza @gurneyalex don't understand runbot and codecov red bullet ... some help ?

@etobella
Copy link
Member

etobella commented Oct 25, 2017

The problem with the runbot is that your code raises a WARNING:
2017-10-24 20:44:42,319 151 WARNING openerp_test odoo.addons.base.ir.ir_ui_view: Error-prone use of @class in view res.partner.kanban.contact (): use the hasclass(*classes) function to filter elements by their classes
Codecov error will be solved when some PR are accepted (no initial report can be found). It should not be a major problem.

@njeudy
Copy link
Author

njeudy commented Oct 25, 2017

Ok will change this :) thanks @etobella

@njeudy njeudy changed the title 11.0 mig partner contact in several companies [11.0] mig partner contact in several companies Oct 27, 2017
@njeudy njeudy force-pushed the 11.0-mig-partner_contact_in_several_companies branch from 02f48bd to 0e5b0e3 Compare October 27, 2017 13:15
@njeudy
Copy link
Author

njeudy commented Oct 27, 2017

  • Ok xpath expression changed
  • And squash transbot commit

@njeudy
Copy link
Author

njeudy commented Oct 30, 2017

@pedrobaeza all green :) ready for review ?

@pedrobaeza
Copy link
Member

Well, I'm not directly interested in this module, so let's wait for other interested reviewers. Any way, I think if you rebase over current branch, you will get coverage, which is a good indicator also.

@njeudy
Copy link
Author

njeudy commented Oct 31, 2017

@pedrobaeza ok no probleme will rebase. thanks

@njeudy njeudy force-pushed the 11.0-mig-partner_contact_in_several_companies branch from 0e5b0e3 to 345ccfa Compare October 31, 2017 13:19
@pedrobaeza pedrobaeza added this to the 11.0 milestone Oct 31, 2017
@njeudy
Copy link
Author

njeudy commented Nov 7, 2017

@yajo @pedrobaeza what should I do to pass codecov ? it is just 10.0 to 11.0 migration no improvement or new function ...

@yajo
Copy link
Member

yajo commented Nov 8, 2017

In this case codecov indicates that some merged code is not being tested. If you hit "Details > Diff", you'll see some red lines indicating that. You'd have to add some test for those lines to go over the target patch coverage (96.34%) and to avoid lowering the current project coverage ratio.

It would be a much appreciated effort, although I wouldn't require such thing IMHO.

@njeudy njeudy force-pushed the 11.0-mig-partner_contact_in_several_companies branch 2 times, most recently from 49f3034 to 1b8a746 Compare November 8, 2017 20:51
@njeudy
Copy link
Author

njeudy commented Nov 8, 2017

@yajo done ;)

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Cool! Please squash all migration-related commits into one for the final merge 😊

@njeudy njeudy force-pushed the 11.0-mig-partner_contact_in_several_companies branch from 92e79d1 to 43e65d6 Compare November 13, 2017 21:33
@njeudy
Copy link
Author

njeudy commented Nov 13, 2017

@yajo like that ?

@yajo
Copy link
Member

yajo commented Nov 14, 2017

With 867d552 too

@njeudy
Copy link
Author

njeudy commented Nov 15, 2017

@yajo : did not understand why this commit is shown, in my branch all commit are squashed in one.
https://github.com/njeudy/partner-contact/commits/11.0-mig-partner_contact_in_several_companies ... don't know how to do.

@yajo
Copy link
Member

yajo commented Nov 15, 2017

@njeudy don't worry, let me show you. Look at the commits in the PR:

captura el 2017-11-15 a las 14 36 21

I highlighted all of the remaining translation commits.

The normal flow would be:

git checkout 11.0-mig-partner_contact_in_several_companies
git rebase --interactive 11.0

That should open the editor you have configured and show all the list of commits from your branch.

They should appear in this fashion:

pick <commit-sha> <commit-msg>
[...]

Now you should cut all those lines that I marked in red and move them to the bottom, just before your migration commit, and mark as fixup all of them except the first one. The end of the commits list should look something similar to this:

pick e112903 Create partner_contact_personal_information_page.
[...]
# Pick 1st translation commit
pick 4d5d348 OCA Transbot updated translations from Transifex 
# Fixup the rest of translation commits
fixup e639882 OCA Transbot updated translations from Transifex
# ...
fixup 2024a3e OCA Transbot updated translations from Transifex
# Your migration commit at the end:
pick 43e65d6 [MIG] V11 migration

Then save and exit. Git will start picking all code commits, then it will pick 1st translation commit and fixup (squash discarding commit message) all of other translation commits in that one, then it will pick your migration commit and will let a clean branch! There's chance that merge conflicts appear, in such case install meld and, when git rebase stops, do:

git mergetool
# now, using meld, fix conflicts on middle pane. Then:
git rebase --continue

If you even use that fixup system to remove the duplicate commits from your branch, that would be even more awesome, but this should be just fine with that.

@njeudy njeudy force-pushed the 11.0-mig-partner_contact_in_several_companies branch 2 times, most recently from 372788d to 6ae7320 Compare November 18, 2017 17:44
@njeudy
Copy link
Author

njeudy commented Nov 18, 2017

@yajo thanks for your help, now it should be ok and have learn a lot if git rebase tricks :)

sbidoul and others added 18 commits December 5, 2017 21:56
- Remove Domain Error on contact view.
- Additionally, improve behaviour of personal contact info page.
- Change to README.
- No longer dependent on partner_contact_personal_information_page
- Added tests for ir_actions
- Include extra demo data.
- Increase test coverage.
- Remove birthdate from demo data.
* [MIG] inital work to 10.0

* [FIX] restore computed field

- select=1 is not usable anymore ...

* [FIX] convert _fields_sync to new api

* [FIX] restore form . import statement

* [FIX] readme with last template.

* [FIX] import statement.

* [FIX] check all tests

* [FIX] Flake8 errors fixes

* [FIX] flake 8

* [FIX] wrong obj call self -> self.contact_id

* [FIX] typo

* [FIX] flake 8

* [FIX] add index on contact_type

* [fix] use in statement on test

* [FIX] update base action with new context

* [FIX] test fixes

* [FIX] test work

* [FIX] flake 8 over indent

* [FIX] fixes on lastest comments

- better readability
- remove <data></data> tags

* [FIX] model that same pattern for _fields_sync

- add test ensure_once()

* [FIX] remove last data xml tags

* [FIX] fix action test

* [FIX] typo in import statement

* [FIX] remove logger from tests

* [FIX] new flake8 compute method name for OCA

* [FIX] minor typo

* [FIX] partner_contact_in_several_companies: remove contributor key in manifest

* [FIX] partner_contact_in_several_companies: residual onchange on partner view

* Revert "[FIX] partner_contact_in_several_companies: remove contributor key in manifest"

This reverts commit 56e511a.

* [FIX] partner_contact_personal_information_page: clean manifest and authors

* [FIX] partner_contact_in_several_companies: required = True not needed
Really I'm just renaming partner_contact_base.
The new name is more self-explanatory.
OCA#315)

[MIG] Migration partner_contact_birthdate partner_contact_gender partner_contact_personal_information_page to v10
- fax field removed from res.partner
- 2to3 on all py files
- add contributor and update README
- use hasclass in place of @Class in xpath expr
- improve tests

[FIX] typo and clean README
@njeudy njeudy force-pushed the 11.0-mig-partner_contact_in_several_companies branch from 5c2bb83 to 0362443 Compare December 5, 2017 20:59
@njeudy
Copy link
Author

njeudy commented Dec 5, 2017

@simahawk Ok done squash commit from same user in one

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

LGTM. I'd squash commits like "make module uninstallable" and alike, just because they are useless in this context. But is not a blocker and we don't have guidelines for this ;)

@njeudy
Copy link
Author

njeudy commented Dec 11, 2017

@pedrobaeza @simahawk @yajo Ok to merge ?

@yajo yajo merged commit da905ca into OCA:11.0 Dec 12, 2017
@pedrobaeza
Copy link
Member

This has broken Travis. I have already fixed one issue, but it seems that there are more. This can't be installed. Please check https://travis-ci.org/OCA/partner-contact/jobs/317716741#L780

@pedrobaeza
Copy link
Member

Please fix Travis after merging this.

@pedrobaeza
Copy link
Member

Solved thanks to @drdran

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.

10 participants