Skip to content

project and project_issue review to 10.0 - #848

Merged
hbrunn merged 1 commit into
OCA:10.0from
DarioLodeiros:patch-1
Jul 26, 2017
Merged

project and project_issue review to 10.0#848
hbrunn merged 1 commit into
OCA:10.0from
DarioLodeiros:patch-1

Conversation

@DarioLodeiros

@DarioLodeiros DarioLodeiros commented May 29, 2017

Copy link
Copy Markdown
Member

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@omar7r omar7r mentioned this pull request May 29, 2017
Closed
cubells
cubells approved these changes May 29, 2017
View reviewed changes

@cubells cubells left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍
LGTM

pedrobaeza
pedrobaeza requested changes May 30, 2017
View reviewed changes

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These 3 noupdate record rules need to be removed (2 in project and 1 in project_issue):

DEL ir.rule: project.portal_project_rule
DEL ir.rule: project.portal_task_rule
DEL ir.rule: project_issue.portal_issue_rule

@DarioLodeiros
DarioLodeiros force-pushed the patch-1 branch 2 times, most recently from 16fe3a2 to 54c4ab3 Compare May 30, 2017 09:49
@DarioLodeiros

DarioLodeiros commented May 30, 2017

Copy link
Copy Markdown
Member Author

@pedrobaeza it is done

@pedrobaeza

pedrobaeza commented May 30, 2017

Copy link
Copy Markdown
Member

You are removing the XML-IDs, not the rules itself. Do first self.env.ref('xml_id').unlink

@DarioLodeiros

DarioLodeiros commented May 30, 2017

Copy link
Copy Markdown
Member Author

@pedrobaeza it is done

NL66278
NL66278 reviewed May 30, 2017
View reviewed changes
Comment thread addons/project/migrations/10.0.1.1/pre-migration.py Outdated

@NL66278 NL66278 May 30, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do not put this comment here. Actually not anywhere as it is very clear from the code what is done.

NL66278
NL66278 reviewed May 30, 2017
View reviewed changes
Comment thread addons/project_issue/migrations/10.0.1.0/pre-migration.py Outdated

@NL66278 NL66278 May 30, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same for this - do not put comment for something that is obvious from the code.

@hbrunn hbrunn mentioned this pull request Jun 6, 2017
Closed
hbrunn
hbrunn requested changes Jun 6, 2017
View reviewed changes
Comment thread addons/project/migrations/10.0.1.1/pre-migration.py Outdated

@hbrunn hbrunn Jun 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

those are actually not necessary, because the unlinks above will also clean up xmlids pointing to the records in question: https://github.com/OCA/OCB/blob/10.0/odoo/models.py#L3418-L3420
And xmlids not in use any more are also cleaned up by the system

@gfcapalbo gfcapalbo Jul 26, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

query has been removed by dario

@MiquelRForgeFlow

MiquelRForgeFlow commented Jun 12, 2017

Copy link
Copy Markdown
Contributor

Please, rebase the PR ☺️

@pedrobaeza

pedrobaeza commented Jun 21, 2017

Copy link
Copy Markdown
Member

Are you going to continue with this PR?

@DarioLodeiros

DarioLodeiros commented Jun 21, 2017

Copy link
Copy Markdown
Member Author

Yes! Sorry, but I had problem to understand rebase...
and, the delete is not necesary?

@pedrobaeza

pedrobaeza commented Jun 21, 2017

Copy link
Copy Markdown
Member

I think you have made things the contrary. You should remove records, not ir_model_data

@DarioLodeiros
DarioLodeiros force-pushed the patch-1 branch 2 times, most recently from 980e9b9 to ce56605 Compare June 21, 2017 16:56
@DarioLodeiros

DarioLodeiros commented Jun 21, 2017

Copy link
Copy Markdown
Member Author

@pedrobaeza Is it ok??

@pedrobaeza

pedrobaeza commented Jun 21, 2017

Copy link
Copy Markdown
Member

I'm afraid not. You have to remove the SQL instruction that removes ir_model_data, and keep these instructions:

env.ref('project.portal_project_rule').unlink()
env.ref('project.portal_task_rule').unlink()
env.ref('project.action_client_project_menu').unlink()

@DarioLodeiros

DarioLodeiros commented Jun 22, 2017

Copy link
Copy Markdown
Member Author

Ok! Sorry for the confusion, now I see it... but, not the same thing here??
https://github.com/DarioLodeiros/OpenUpgrade/blob/patch-1/addons/project_issue/migrations/10.0.1.0/pre-migration.py#L13-L17

@pedrobaeza

pedrobaeza commented Jun 22, 2017

Copy link
Copy Markdown
Member

Of course, we are talking to do the same on both.

@DarioLodeiros

DarioLodeiros commented Jun 22, 2017

Copy link
Copy Markdown
Member Author

Ok!
Thanks for the help!! 😃

MiquelRForgeFlow
MiquelRForgeFlow requested changes Jun 22, 2017
View reviewed changes
Comment thread addons/project/migrations/10.0.1.1/pre-migration.py Outdated

@MiquelRForgeFlow MiquelRForgeFlow Jun 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please, delete the two +s

@DarioLodeiros DarioLodeiros Jun 22, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ups! Sorry! It's done

@pedrobaeza

pedrobaeza commented Jun 22, 2017

Copy link
Copy Markdown
Member

There are lint errors:

./addons/project_issue/migrations/10.0.1.0/pre-migration.py:10:5: F841 local variable 'cr' is assigned to but never used
./addons/project_issue/migrations/10.0.1.0/pre-migration.py:13:1: W391 blank line at end of file

MiquelRForgeFlow
MiquelRForgeFlow approved these changes Jun 26, 2017
View reviewed changes

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@pedrobaeza

pedrobaeza commented Jun 30, 2017

Copy link
Copy Markdown
Member

As @hbrunn's concerns are covered, I merge.

@gfcapalbo

gfcapalbo commented Jul 26, 2017

Copy link
Copy Markdown

I have cloned dario's branch and verified all your suggestions and lint problems are solved. What's stopping the merge?

@hbrunn

hbrunn commented Jul 26, 2017

Copy link
Copy Markdown
Member

nothing

@hbrunn
hbrunn merged commit 261928c into OCA:10.0 Jul 26, 2017
@pedrobaeza

pedrobaeza commented Jul 26, 2017

Copy link
Copy Markdown
Member

I forgot to merge...

@pedrobaeza pedrobaeza mentioned this pull request Aug 25, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hbrunn hbrunn hbrunn approved these changes
@cubells cubells cubells approved these changes
@pedrobaeza pedrobaeza pedrobaeza approved these changes
@MiquelRForgeFlow MiquelRForgeFlow MiquelRForgeFlow approved these changes
+2 more reviewers
@NL66278 NL66278 NL66278 left review comments
@gfcapalbo gfcapalbo gfcapalbo left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs fixing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@DarioLodeiros @pedrobaeza @MiquelRForgeFlow @gfcapalbo @hbrunn @cubells @NL66278