Skip to content

[MIG] 9.0 - Migrate 'base_user_role'#641

Merged
pedrobaeza merged 5 commits into
OCA:9.0from
osiell:9.0-mig-base_user_role
Dec 16, 2016
Merged

[MIG] 9.0 - Migrate 'base_user_role'#641
pedrobaeza merged 5 commits into
OCA:9.0from
osiell:9.0-mig-base_user_role

Conversation

@sebalix

@sebalix sebalix commented Dec 9, 2016

Copy link
Copy Markdown
Contributor

@lasley here it is

…CA#608)

* [ADD] New module 'base_user_role'

* [FIX] base_user_role - Review

* [FIX] base_user_role - Review s/is_active/is_enabled/

* [FIX] base_user_role - Review s/is_active/is_enabled/

* [IMP] base_user_role - Translations updated (template + FR)

* [FIX] base_user_role - Lint
@sebalix sebalix changed the title [9.0] Migrate 'base_user_role' [MIG] 9.0 - Migrate 'base_user_role' Dec 9, 2016

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

Thanks @sebalix

Comment thread base_user_role/data/ir_cron.xml Outdated
<?xml version="1.0" encoding="utf-8"?>
<!-- Copyright 2016 ABF OSIELL <http://osiell.com>
License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). -->
<odoo>

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.

odoo noupdate="1"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,25 @@
# -*- coding: utf-8 -*-

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.

Past version migration can be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<field name="inherit_id" ref="base.view_users_form"/>
<field name="arch" type="xml">
<notebook position="inside">
<page string="Roles">

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.

What would you think about putting the Roles as first page in the notebook, ie before the standards groups page. As admins using this module will almost never look at the detailed groups, it's probably better to show roles first.

line_ids = fields.One2many(
'res.users.role.line', 'role_id', string=u"Users")
user_ids = fields.One2many(
'res.users', string=u"Users", compute='_compute_user_ids')

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.

Since roles are groups too, it would be nice to put them in their own category so they are more easily distinguished from regular groups in the groups configuration page.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is exactly how we do when declaring our roles as XML records, it could be done by default by associating them to a 'User roles' category indeed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sbidoul Done

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

@sebalix excellent module! I made a couple of ergonomy suggestions but as this is a migration PR, 👍

group_category_id = fields.Many2one(
related='group_id.category_id',
default=lambda cls: cls.env.ref(
'base_user_role.ir_module_category_role').id)

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.

Cool, thanks for adding this. Would it work too with a simple override category_id = fields.Many2one(default=...)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not work without the related part. About the field's name, I don't know what is best.

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.

so weird to me... anyway it works fine, thanks!

@sebalix

sebalix commented Dec 16, 2016

Copy link
Copy Markdown
Contributor Author

2 thumbs up and 5 days, ready to merge I think ;) I'll start the migration to v10 soon.

@pedrobaeza

Copy link
Copy Markdown
Member

Can you please squash your fixes in the review process commits?

sebalix and others added 4 commits December 16, 2016 16:12
[IMP] base_user_role - Replace '<openerp>' tags by '<odoo>' + Remove useless '<data>' tags + Reindent XML content

[FIX] base_user_role - Fix noupdate declaration

[REM] base_user_role - Remove old migration script
@sebalix
sebalix force-pushed the 9.0-mig-base_user_role branch from 12ce852 to 167d999 Compare December 16, 2016 15:14
@sebalix

sebalix commented Dec 16, 2016

Copy link
Copy Markdown
Contributor Author

@pedrobaeza Done, I kept the commits from @sbidoul and the last one related to the default module category as they are new features which could be backported on the 8.0 version.

@pedrobaeza

Copy link
Copy Markdown
Member

Thanks for the comprehensive squashing! I'll merge when CIs finish.

@pedrobaeza
pedrobaeza merged commit 7cf4639 into OCA:9.0 Dec 16, 2016
@pedrobaeza pedrobaeza mentioned this pull request Dec 16, 2016
59 tasks
@sebalix
sebalix deleted the 9.0-mig-base_user_role branch December 18, 2016 09:57
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (10.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants