-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[9.0] [ADD] Base User Role History #1683
Conversation
2fd7943
to
fe9cafd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baimont Please add a roadmap for role deletion. If a role is deleted, the history line too. That may be improved later (maybe with a char field).
fe9cafd
to
669e123
Compare
@ThomasBinsfeld thanks, done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice module! Small comments
<field name="new_is_enabled"/> | ||
</group> | ||
</sheet> | ||
<div class="oe_chatter"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed maybe? (we're not inheriting from mail.thread
)
<field name="arch" type="xml"> | ||
<form create="0" edit="0" delete="0"> | ||
<header> | ||
</header> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<header>
section empty, can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it if one day someone wants to put something in header, he'll just have to add a header position="inside".
'website': 'https://github.com/OCA/server-tools', | ||
'depends': [ | ||
# Odoo | ||
'base', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to depend on base
<field name="perm_create" eval="0"/> | ||
<field name="perm_write" eval="0"/> | ||
<field name="perm_unlink" eval="0"/> | ||
</record> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we create the ir.model.access through CSV, but not blocking.
'views/base_user_role_line_history.xml', | ||
'views/res_users.xml', | ||
], | ||
'demo': [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty keys can be removed.
669e123
to
7385fa5
Compare
Hello @sebalix and thanks for your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG
@baimont can you add
in the |
7385fa5
to
155e48a
Compare
Travis is red but I don't think it's related to my changes. |
Doesn't seem related to your PR, there are some "SQL injection" issues in other modules detected by pylint-odoo. |
So merge is blocked because of other modules? |
@baimont if you want to speed up the process don't hesitate to fix all other lints in a separated commit. Unless someone is ok to merge it as is (ping @pedrobaeza ) |
Sorry, we can't merge with a red status |
Long live the PR 🤷♂️ |
0b7e7b6
to
56d11de
Compare
The 9.0 branch is red since at least July 2018. |
Travis is red, please resolve |
56d11de
to
03b38ee
Compare
Travis is red. |
Not anymore :-D |
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
@thomaspaulb your merge command was aborted due to failed check(s), which you can inspect on this commit of 9.0-ocabot-merge-pr-1683-by-thomaspaulb-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
No description provided.