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

[12.0][MIG] mail_activity_team #340

Closed
wants to merge 7 commits into from
Closed

[12.0][MIG] mail_activity_team #340

wants to merge 7 commits into from

Conversation

MiquelRForgeFlow
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow commented Dec 3, 2018 •
edited
Loading

Standard migration of #330.

Depends of:

@pedrobaeza pedrobaeza added this to the 12.0 milestone Dec 3, 2018
@AdriaGForgeFlow AdriaGForgeFlow force-pushed the 12.0-mig-mail_activity_team branch 2 times, most recently from 67820f2 to 81747b5 Compare December 24, 2018 08:43
@HviorForgeFlow
Copy link
Member

@ageficent @mreficent dependency is already merge, can you rebase for oca_dependencies conflict?

@MiquelRForgeFlow
Copy link
Contributor Author

@hveficent done

HviorForgeFlow
HviorForgeFlow approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@HviorForgeFlow HviorForgeFlow left a comment

Choose a reason for hiding this comment

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

Functionaly review LGTM 👍

tbaden
tbaden approved these changes Apr 21, 2019
View reviewed changes
Copy link
Member

@tbaden tbaden left a comment

Choose a reason for hiding this comment

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

code review

if you like apply cosmetical changes

mail_activity_team/security/mail_activity_team_security.xml
<?xml version="1.0" encoding="utf-8"?>
<odoo noupdate="1">

<record id="mail_activity_rule_my_team" model="ir.rule">
Copy link
Member

Choose a reason for hiding this comment

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

cosmetical change: use original id

prefer naming the inherited view after the original over using a name which follows the guidelines.

<record id="original_id" model="ir.ui.view">
    <field name="inherit_id" ref="original_module.original_id"/>
    ...
</record>

mail_activity_team/views/res_users_views.xml
<?xml version="1.0"?>
<odoo>
<!-- Update user form !-->
<record id="view_users_form_activity_teams" model="ir.ui.view">
Copy link
Member

Choose a reason for hiding this comment

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

cosmetical change: use original id

prefer naming the inherited view after the original over using a name which follows the guidelines.

<record id="original_id" model="ir.ui.view">
    <field name="inherit_id" ref="original_module.original_id"/>
    ...
</record>

@emagdalenaC2i emagdalenaC2i mentioned this pull request Jun 1, 2019
Closed
24 tasks
bealdav
bealdav approved these changes Jul 7, 2019
View reviewed changes
Copy link
Member

@bealdav bealdav 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.

Just put in production.
LGTM for me
Thanks for this contribution

@bealdav
Copy link
Member

bealdav commented Jul 10, 2019

@mreficent please could you fix @tbaden remarks ?
Then could be merged

@MiquelRForgeFlow
Copy link
Contributor Author

I wouldn't call "fix" to those cosmetic changes. They are optional.

@bealdav
Copy link
Member

bealdav commented Jul 10, 2019

Ok. Maybe we can merge, then ?

@sebastienbeau
Copy link
Member

/ocabot merge

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Rebased to 12.0-ocabot-merge-pr-340-by-sebastienbeau-bump-no, awaiting test results.

@OCA-git-bot
Copy link
Contributor

It looks like something changed on 12.0 in the meantime. Let me try again (no action is required from you).
Rebased to 12.0-ocabot-merge-pr-340-by-sebastienbeau-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 11, 2019
@OCA-git-bot
Merge PR #340 into 12.0
978145e 
Signed-off-by sebastienbeau
@OCA-git-bot
Copy link
Contributor

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

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 12.0.

@MiquelRForgeFlow MiquelRForgeFlow deleted the 12.0-mig-mail_activity_team branch July 11, 2019 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bealdav bealdav bealdav approved these changes

@tbaden tbaden tbaden approved these changes

@HviorForgeFlow HviorForgeFlow HviorForgeFlow approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 needs review
Projects
None yet
Milestone
12.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@MiquelRForgeFlow @HviorForgeFlow @bealdav @sebastienbeau @OCA-git-bot @tbaden @hbrunn @pedrobaeza @JordiBForgeFlow @AdriaGForgeFlow