Skip to content

Conversation

@mohamedhagag
Copy link

this is a fix for issue:
#1227
based on the PR by akretion for 8 and 9 here:
#519

this is a fix for #1227 based on the missing PR by akretion for 8 and 9 .
#519
avoid server errors if empty value for config params.
fix for getting current value
I'm sure there can be a better way to get them.
@pedrobaeza
Copy link
Member

Please change module version number according https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#version-numbers

And check travis status

@mohamedhagag
Copy link
Author

Version number change will create another PR, is this OK ?

Regarding the travis status I can't find errors related to this module, it complains about other modules like auth_brute_force , auth_user_case_insensitive and other modules.
What should I do ?

Sorry but I'm new in contributing to OSS projects using github.

@pedrobaeza
Copy link
Member

@mohamedhagag no, adding version number doesn't create other PR if you do it through git, not via GitHub interface.

And the Travis errores are yours:

auth_admin_passkey/models/res_config.py:8:1: E302 expected 2 blank lines, found 1
auth_admin_passkey/models/res_config.py:17:63: E231 missing whitespace after ','
auth_admin_passkey/models/res_config.py:32:62: E231 missing whitespace after ','

@mohamedhagag
Copy link
Author

@pedrobaeza would you plz chk and merge.

@pedrobaeza
Copy link
Member

This is not how it works. Check https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md for the general rules. It requires at least 2 reviews, and a way to get reviews is to make another reviews and ask in return to review yours.


auth_admin_passkey_send_to_user = fields.Boolean(
string='Send email to user.',
help="""When the administrator use his password to login in """
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

  • you moved a lot of code, that make the diff quite hard to review. I don't see the reason of that ?
  • I think adding triple quote is unnecessary.

Other it looks great for me !

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it is a good idea to get rid of the triple quotes. They were introduced in the fix for 8.0.

@NL66278
Copy link
Contributor

NL66278 commented Jan 8, 2019

I think the latest two commits, using the string 'False' instead of an empty string when setting the parameters, and then testing for string 'True' or '1' when getting the parameters is not needed. The second commit fixes an error introduced by the first commit. Setting the parameter to an empty string effectively sets it to a False or rather falsey value. So original code as now existing in 8.0 just should work IMHO.

@dek-odoo
Copy link

dek-odoo commented May 4, 2019

are there any updates ? The issue is still there.

@gurneyalex
Copy link
Member

superseeded by #1720

@gurneyalex gurneyalex closed this Dec 12, 2019
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (16.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.

6 participants