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.

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

[MIG][11.0] server_environment_ir_config_parameter #8

Merged
merged 11 commits into from
Jan 24, 2018

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Jan 16, 2018

replaces #5

NOTE: I used git filter-branch --prune-empty --subdirectory-filter ... to move the module from server-tools which ended up in the root of the repo. I moved it to the right place in the migration commit.
I don't think it's a problem tho.

sbidoul and others added 10 commits January 16, 2018 15:15
… values

This is much more robust that raising an error, and let modules
load ir.config_parameter from xml data files, while still enforcing
values from the config files.
Odoo loads modules that contain static directory, so it wanted to load server_environment_files but it does not exist
Without this sudo get_param would fail when the first user reading a parameter that has changed in the configuration file does not have write access to system parameters.
@simahawk
Copy link
Contributor Author

@sbidoul ping :)

Copy link
Member

@TDu TDu left a comment

Choose a reason for hiding this comment

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

Thank you for taking over. LGTM

@simahawk simahawk force-pushed the server_environment_ir_config-MIG branch from 4b2f277 to 9ae7ca0 Compare January 16, 2018 15:55
@pedrobaeza
Copy link
Member

You can use git am the same as in migration guide. The only difference is the remotes you configure and put in the comment.

@simahawk
Copy link
Contributor Author

@pedrobaeza funny thing is that I tried that way and it didn't worked... but I guess I messed up w/ remotes :/
I'll try it again.

@pedrobaeza
Copy link
Member

Well, as you already have it, no need to try more, but I pointed out only as reference.

Copy link
Member

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

Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

LGTM
I only have a question.

.travis.yml Outdated
@@ -10,6 +10,7 @@ addons:
packages:
- expect-dev # provides unbuffer utility
- python-lxml # because pip installation is slow
- unixodbc-dev

Choose a reason for hiding this comment

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

Why is it required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thierry added it to fix tests on py3. No traceback tho. I removed it, let's see how travis behaves...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leemannd tests pass anyway. Tnx 👍

@@ -0,0 +1,8 @@
<odoo>
<data>

Choose a reason for hiding this comment

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

Isn't this useless if there is no noupdate?

Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

Non blocking comment.
LGTM! Code review only

Copy link

@mpanarin mpanarin left a comment

Choose a reason for hiding this comment

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

other than small things
LGTM


@api.model
def get_param(self, key, default=False):
value = super(IrConfigParameter, self).get_param(key, default=None)

Choose a reason for hiding this comment

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

supers can be reduced to super().foo() in python3 )


class TestEnv(common.TransactionCase):

def setUp(self):

Choose a reason for hiding this comment

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

Isn't setupclass just better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, if you don't do anything :)
setUpClass must be used if you are creating several records that are used by each test.

@simahawk simahawk force-pushed the server_environment_ir_config-MIG branch from 493a474 to fff9b65 Compare January 24, 2018 11:38
@simahawk simahawk force-pushed the server_environment_ir_config-MIG branch from fff9b65 to ed57158 Compare January 24, 2018 11:39
@guewen guewen merged commit bc83dd5 into OCA:11.0 Jan 24, 2018
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.

10 participants