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

[13.0][MIG] partner_priority #870

Merged
merged 6 commits into from
Apr 21, 2020
Merged

[13.0][MIG] partner_priority #870

merged 6 commits into from
Apr 21, 2020

Conversation

newtratip
Copy link
Member

@newtratip newtratip commented Mar 17, 2020 •
edited
Loading

Ready for review.

patrickrwilson and others added 5 commits March 17, 2020 18:30
@patrickrwilson @newtratip
[ADD] partner_priority module
be73641 
This module adds a priority field to contacts which can be configured within the configuration menu.

[UPD] LINT errors

fixed lint error, renamed test py file, added missing readme

[UPD] Removed unused file

removed incorrect py file

[FIX] wrong sequence val in test

Remove Test

[FIX] Data File Sequence

Fixed wrong sequence field name on data file.

[UPD] noupdate

Removed <data> element and moved noupdate into Odoo as suggested by reviewers.

[UPD] Reviewer Changes

Changes based on reviewer's suggestions

[UPD] Additional Review Changes

Additional changes requested from reviewers

[UPD] Sequence

Moved next sequence out of create method and into field def.

[UPD] Review Change

[FIX] LINT
@OCA-git-bot @newtratip
[UPD] README.rst
bc31070
@oca-travis @newtratip
[UPD] Update partner_priority.pot
8516f2b
@OCA-git-bot @newtratip
[UPD] README.rst
6b8f106
@newtratip
[IMP] partner_priority: black, isort
11f275b
@OCA-git-bot
Copy link
Contributor

Hi @patrickrwilson,
some modules you are maintaining are being modified, check this out!

@newtratip newtratip mentioned this pull request Mar 17, 2020
Open
47 tasks
lfreeke
lfreeke approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@lfreeke lfreeke left a comment

Choose a reason for hiding this comment

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

Functional test 👍

patrickrwilson
patrickrwilson approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@patrickrwilson patrickrwilson left a comment

Choose a reason for hiding this comment

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

LGTM

@newtratip
Copy link
Member Author

@Saran440 @ps-tubtim Could you review this ?

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

NL66278
NL66278 reviewed Mar 27, 2020
View reviewed changes
partner_priority/models/partner_priority.py

name = fields.Char(string="Priority", required=True)
description = fields.Text(required=True)
sequence = fields.Integer(
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this field set from an ir.sequence instance looks a little bit to over-engineering to me.

NL66278
NL66278 approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

Some remarks. But not blocking as this is a migration.

@NL66278
Copy link
Contributor

NL66278 commented Apr 21, 2020

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Hi @NL66278. Your command failed:

Required option bumpversion_mode for command merge. Possible values : major, minor, patch, nobump.

Ocabot commands

  • ocabot merge major|minor|patch|nobump

More information

@NL66278
Copy link
Contributor

NL66278 commented Apr 21, 2020

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-870-by-NL66278-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit eb4ef01 into OCA:13.0 Apr 21, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 9f4aa72. Thanks a lot for contributing to OCA. ❤️

@rousseldenis
Copy link
Sponsor Contributor

@NL66278 The ocabot merge command has changed. The nobump option does not generate wheels for pypi. Even for migration PRs now, use major/minor/patch.

@NL66278
Copy link
Contributor

NL66278 commented Apr 22, 2020

@rousseldenis In the past when I used major on a migration merge, I was told to leave out the major/minor/patch altogether. Recently the version argument became required, but the nobump was added, so I used that. So what am I supposed to use on a migration merge? major would seem to me the most logical...

@rousseldenis
Copy link
Sponsor Contributor

Following the conversation in mailing list, the idea is to use patch. So, at the end, the module would have 13.0.1.0.1. But the wheels should be generated.

@newtratip newtratip deleted the 13.0-mig-partner_priority branch May 6, 2020 13:28
@newtratip newtratip mentioned this pull request Nov 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NL66278 NL66278 NL66278 approved these changes

@lfreeke lfreeke lfreeke approved these changes

@patrickrwilson patrickrwilson patrickrwilson approved these changes

@ps-tubtim ps-tubtim ps-tubtim approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@newtratip @OCA-git-bot @NL66278 @rousseldenis @lfreeke @patrickrwilson @ps-tubtim @oca-travis