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] auth_user_case_insensitive: mig #2

Merged
merged 26 commits into from
Apr 11, 2022

Conversation

Herqs
Copy link

@Herqs Herqs commented Mar 24, 2022

[BRANCH] mig/1048-user-case-insensitive-hml

oca/server-auth PR: OCA#361

Comment on lines 45 to 73
def test_login_login_is_lowercased(self):
"""It should verify the login is set to lowercase on login"""
# TODO Write a proper test, that does not use
# the admin user, but instead uses the one
# in `_new_record` method.
res_id = self.model_obj._login(
self.env.registry.db_name,
"aDMIn",
"admin",
{"interactive": True},
)
rec_id = self.model_obj.search([("login", "=", "admin")])
self.assertEqual(
rec_id.id,
res_id,
"Login with with uppercase chars was not \
successful",
)
Copy link
Author

Choose a reason for hiding this comment

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

Could not make this test work with the fresh user, so I used the default admin credentials.
Link to original tests: https://github.com/OCA/server-auth/blob/fd4947f04ffd8d7d5b4cf63c06a8dea71df40722/auth_user_case_insensitive/tests/test_res_users.py#L48

@Herqs Herqs requested review from oerp-odoo and SButko March 24, 2022 07:43
Copy link

@SButko SButko left a comment

Choose a reason for hiding this comment

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

Isn't there a way to see what exact changes was made? I mean, initial commit and actual changes could be included in separate commits, because now it's not clear what the changes are.

@Herqs Herqs marked this pull request as draft March 25, 2022 09:57
Ted Salmon and others added 25 commits March 25, 2022 13:21
…f `_login`

* Update code and tests to override `_login` method
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
Currently translated at 100.0% (2 of 2 strings)

Translation: server-auth-12.0/server-auth-12.0-auth_user_case_insensitive
Translate-URL: https://translation.odoo-community.org/projects/server-auth-12-0/server-auth-12-0-auth_user_case_insensitive/pt_BR/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: server-auth-13.0/server-auth-13.0-auth_user_case_insensitive
Translate-URL: https://translation.odoo-community.org/projects/server-auth-13-0/server-auth-13-0-auth_user_case_insensitive/
@Herqs Herqs force-pushed the mig/1048-user-case-insensitive-hml branch 2 times, most recently from 4b17643 to d44a4ea Compare March 28, 2022 11:33
@Herqs Herqs marked this pull request as ready for review April 4, 2022 06:14
@Herqs Herqs force-pushed the mig/1048-user-case-insensitive-hml branch from d44a4ea to e1f7327 Compare April 6, 2022 12:47
users.append(login)
else:
raise ValidationError(
_("Conflicting user logins exist for `%s`") % login
Copy link

Choose a reason for hiding this comment

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

Suggested change
_("Conflicting user logins exist for `%s`") % login
_("Conflicting user logins exist for `%s`", login)

#. module: auth_user_case_insensitive
#: model:ir.model,name:auth_user_case_insensitive.model_res_users
msgid "Users"
msgstr "用户"
Copy link

Choose a reason for hiding this comment

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

Could include lt.po

Copy link
Author

Choose a reason for hiding this comment

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

Added lt.po, not sure how great my translation is though, as there is no good translation for the phrase case sensitive.

@Herqs Herqs force-pushed the mig/1048-user-case-insensitive-hml branch from e1f7327 to 6a01560 Compare April 7, 2022 11:00
@Herqs Herqs requested a review from SButko April 7, 2022 11:05
#: code:addons/auth_user_case_insensitive/hooks.py:0
#, python-format
msgid "Conflicting user logins exist for `%s`"
msgstr "Vartotojams `%s` egzistuoja konfliktuojantys prisijungimai"
Copy link

Choose a reason for hiding this comment

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

That query checks for uniqueness, no?

Suggested change
msgstr "Vartotojams `%s` egzistuoja konfliktuojantys prisijungimai"
msgstr "Vartotojo prisijungimo vardas `%s` jau egzistuoja"

Copy link
Author

Choose a reason for hiding this comment

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

Sort of, it checks if there are 2 usernames that are identical when lowercased, check happens pre-install

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to Egzistuoja keli vartotojai su prisijungimu %s``

#. module: auth_user_case_insensitive
#: model:ir.model.fields,help:auth_user_case_insensitive.field_res_users__login
msgid "Used to log into the system. Case insensitive."
msgstr "Naudojamas prisijungti į sistemą. Nereikšminga ar vesite didžiosiomis ar mažosiomis raidėmis."
Copy link

Choose a reason for hiding this comment

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

Suggested change
msgstr "Naudojamas prisijungti į sistemą. Nereikšminga ar vesite didžiosiomis ar mažosiomis raidėmis."
msgstr "Naudojamas prisijungti į sistemą. Raidžių dydis nesvarbus."

@Herqs Herqs force-pushed the mig/1048-user-case-insensitive-hml branch from 6a01560 to c468a99 Compare April 8, 2022 07:20
@Herqs Herqs requested a review from SButko April 8, 2022 10:51
partner_id = self.env["res.partner"].create(self.partner_vals)
self.vals["partner_id"] = partner_id.id
return self.model_obj.create(self.vals)

def test_login_is_lowercased_on_create(self):
""" It should verify the login is set to lowercase on create """
"""It should verify the login is set to lowercase on create"""

Choose a reason for hiding this comment

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

So if changing docstring, add period at the end, to make it pydocstyle257 compatible. Also docstring should start with a verb for method. Instead of It should verify, use Verify the login is set..

@@ -20,13 +18,13 @@ def setUp(self):
self.model_obj = self.env["res.users"]

def _new_record(self):
""" It should enerate a new record to test with """
"""It should enerate a new record to test with"""

Choose a reason for hiding this comment

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

Is generate I guess, not enerate

@Herqs Herqs force-pushed the mig/1048-user-case-insensitive-hml branch from c468a99 to 22c41d7 Compare April 11, 2022 08:44
@Herqs Herqs force-pushed the mig/1048-user-case-insensitive-hml branch from 22c41d7 to da63b64 Compare April 11, 2022 08:48
@Herqs Herqs merged commit e84cd21 into 15.0-ea Apr 11, 2022
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.