Skip to content

[10.0] [FIX] users_ldap_populate special characters - #1564

Merged
StefanRijnhart merged 6 commits into
OCA:10.0from
r-hede:patch-1
May 3, 2019
Merged

[10.0] [FIX] users_ldap_populate special characters#1564
StefanRijnhart merged 6 commits into
OCA:10.0from
r-hede:patch-1

Conversation

@r-hede

@r-hede r-hede commented Apr 24, 2019

Copy link
Copy Markdown

Hi,

Currently, you can not use special characters in the username, password, or LDAP base.

To fix that, you just have to modify the lines 126 to 130 of the file users_ldap.py like this:

conn.simple_bind_s(conf['ldap_binddn'].encode('utf-8') or '',
                   conf['ldap_password'].encode('utf-8') or '')
results = conn.search_st(conf['ldap_base'].encode('utf-8'), ldap.SCOPE_SUBTREE,
                         ldap_filter.encode('utf8'), None,
                         timeout=60)

Best regards,

Robin Hède.

@NL66278

NL66278 commented Apr 25, 2019

Copy link
Copy Markdown
Contributor

Please fix this error in travis (not caused by your PR, but apparently existing issue):

************* Module users_ldap_populate.models.users_ldap
users_ldap_populate/models/users_ldap.py:90: [C8107(translation-required), CompanyLDAP.action_populate] String parameter on "UserError" requires translation. Use _('Unable to process user with login %s')

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

👍 LGTM, but needs one extra change to satisfy travis.

Comment thread users_ldap_populate/models/users_ldap.py Outdated
@r-hede r-hede changed the title [10.0] users_ldap_populate special characters [10.0] [FIX] users_ldap_populate special characters Apr 25, 2019

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

👍 LGTM

@r-hede

r-hede commented Apr 30, 2019

Copy link
Copy Markdown
Author

Is there any other steps I've to take to get this pull request reviewed and merged?

@NL66278

NL66278 commented Apr 30, 2019

Copy link
Copy Markdown
Contributor

I will ping some people I know. I do not have any special authority in this repository.

@StefanRijnhart @hbrunn

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

@r-hede the best way to get your stuff reviewed is reviewing yourself in the same repository

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@StefanRijnhart

Copy link
Copy Markdown
Member

Couple of questions: Why are there changes in all these translation files? Don't we leave those to the automatic Weblate integration? And, why are there arbitrary whitespace changes? Between those two, I'm having trouble to spot the actual fix. So if you could give me a pointer to that, that would be great.

@StefanRijnhart

Copy link
Copy Markdown
Member

OK, found the actual fix (after reading this PR's description, thanks!) and they warrant the whitespace changes. That leaves the question about the translation files.

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

Here there's a remaining question, so I mark for waiting this way

@NL66278

NL66278 commented May 1, 2019

Copy link
Copy Markdown
Contributor

@StefanRijnhart I looked into the translation files and they contained some wierdness before the changes, mainly having first an empty msgid "" on one line, and then the actual string on the next. So I can imagine @r-hede wanting to rectify this. But maybe he can answer this better himself.

@StefanRijnhart

Copy link
Copy Markdown
Member

@pedrobaeza Can you confirm that translation files should be left to the weblate integration?

@r-hede

r-hede commented May 2, 2019

Copy link
Copy Markdown
Author

Indeed, @NL66278 is right. I deleted the empty spaces at the beginning of many translations which seemed to me useless.

@pedrobaeza

Copy link
Copy Markdown
Member

That's automatically done by OCA Git Bot when merging the PR, but it's not a critical problem to have it here. In the worst case, we'll have an extra commit undoing what is done here.

@StefanRijnhart

Copy link
Copy Markdown
Member

OK, then it's allright!

@StefanRijnhart
StefanRijnhart merged commit 7fbb944 into OCA:10.0 May 3, 2019
Tardo pushed a commit to Tecnativa/server-tools that referenced this pull request Aug 13, 2019
* [10.0] users_ldap_populate special characters
dsolanki-initos pushed a commit to initOS/server-tools that referenced this pull request Feb 6, 2020
* [10.0] users_ldap_populate special characters
dsolanki-initos pushed a commit to initOS/server-tools that referenced this pull request Dec 10, 2020
* [10.0] users_ldap_populate special characters
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (14.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