Gérer la fin de scolarité #9
No reviewers
Labels
No labels
bug
Doing
Good first issue
Question
status -- need review
To Do
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: DGNum/django-allauth-ens#9
Loading…
Reference in a new issue
No description provided.
Delete branch "Evarin/archicubes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Un adapter django allauth pour gérer les comptes plus en scolarité et la possible réutilisation des logins clippers.
TODO :
added 1 commit
cdb9c3c7- Mail address disambiguation, debug, only one ldap queryCompare with previous version
marked the checklist item Problème de l'unicité des adresses mails as completed
added 1 commit
5a78561d- WIP testsCompare with previous version
added 2 commits
dc8873ca- Tests for adapter ready54963e9f- Fix other testsCompare with previous version
added 1 commit
1142db73- Revert "Fix other tests"Compare with previous version
added 1 commit
6fc012cb- Fix my testsCompare with previous version
assigned to @champeno
marked the checklist item Vérifier que tout tient la route as completed
added 2 commits
9c3fc72e- One more test caseb6f5acaa- ReadmeCompare with previous version
added 1 commit
f0a73f6e- Util management command to install longtermadapter + fixesCompare with previous version
marked the checklist item Tester avec LDAP as completed
unmarked as a Work In Progress
s/set on hold/put on hold/
Not going to work in python3.
infos.get('cn', [b''])[0]if the underlying call is supposed to return bytes. Not sure what the API ofldapsays.Why not make this an
OrderedDictdirectly?This parsing code is prone to fail when/if the homeDirectory is not /users// which may happen. You should check:
if len(dirs) == 4 and dirs[1] == 'users'with a comment about the assumptions on the format.Zen of python or something says you should do:
if pmail:instead.Those are
stralready, why the casts?s/Super/super/
also is this supposed to be compatible with py2? if not you can just do
super().pre_social_login(request, sociallogin).Looks like this is stored on the adapter here so it can get reused in
save_userbelow. Is there any guarantee from socialaccount that those two will always be called sequentially (esp. regarding thread safety) and that we won't see data for another account? Can we store that data on thesociallogininstead?I think there may be a subtle bug here if the LDAP server is down because then we could end up creating a new SocialAccount in
pre_social_loginfor a user with annee='00'. Probably safer to re-raise an error.See comment on the
passinget_ldap_infos; we could end up creating a bogus user if the ldap server failed for a transient reason.It looks like this
try..exceptis only supposed to guard the very first line of the block? Can we move theexcepthigher up? In general it is a good thing to maketry..exceptblocks as small as required in order to avoid spurious catches.It looks like this
try..exceptis only supposed to guard the very first line of the block? Can we move theexcepthigher up? In general it is a good thing to maketry..exceptblocks as small as required in order to avoid spurious catches.See comment above; can we store that data on the sociallogin instead of the adapter?
s/o avoid/to avoid/
Also s/wer/were/
Hmmm that's true I had in mind that an
adapterobject was unique to the connexion but it's nonsense. It is way more relevant to put it on thesocialloginindeed.changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
changed this line in version 9 of the diff
added 2 commits
a1671a3d- Fixes from Elarnon's reviewbfc0bb42- Add ldap query counting to testsCompare with previous version
I have to check I don't get errors with ldap in practice but should be resolved
We do want the py2 compatibility
Well, it was actually the intended behaviour for local testing (with no access to the LDAP server), but it may not be relevant to keep it this way. As the LDAP query is done only for new connexion or previously deprecated account, it may be indeed better to block the connexion process. Though, rather than raising an exception here, we could handle that in the
get_usernamefunction: if we have noanneein the LDAP data, we raise an error. This way, it could be customized easily by the user in an "error-free" mode?That's also a valid way of doing things I suppose, but I would return
Nonehere in that case to avoid the possibility of depending on a (possibly bogus)'email'field.Needs
b'/'for py3 I guess :)changed this line in version 10 of the diff
changed this line in version 10 of the diff
added 1 commit
787efe96- LDAP error propagation + README + tox update and correctionsCompare with previous version
Adopted the proposed strategy :
get_ldap_infosreturns an empty dict if no info is available ; the default mail address is generated elsewhere ;get_usernameraises a ValueError if it does not find aanneefield in the LDAP data ; the example website(s version is patched so that it does not fail when no access to the LDAP.Should be fixed (as well as for the
emailfieldWhitespaces
Thanks
You can add a new section
Adapters.And there are many whitespaces in this section.
Can you also separate manually the lines (below, or a few above, 80 cols) ?
Typo:
SociallAccount->SocialAccountI think it's better to give a more specific name to this setting.
CLIPPER_LDAP_SERVER?Is there a reason for this option to be set globally ?
data={}is error-prone and can result in invalid returned value in this case.BTW, this paramater doesn't seem necessary according to the use cases.
Unused vars
I think it's better to store values individually :
data['entrance_year'],data['department_code'](lowerised),data['department'](labelised).Better with doc:
Note that this docstring is falsy at this time, see comment below.
a->old_conn, clearer ?clipper->clipper_uid, clearer ?Should we use a better test ? The username can be changed and it can be pretty hard for the staff to remember not doing this.
Can we test
a.extra_data['entrance_year/annee'] !=ldap_data['entrance_year/annee']` ?[Nitpicking Warning, yeah another one]
I don't know if we need a new SocialAccount, but we can't reuse this one (or shouldn't).
A new file
allauth_ens/utils.pywith (for the beginning, note that _init_ldap, _get_infos_from_ldap and a few others could go there):The same appears below in this file, and also in the
allauth_ens.providers.clipper.providermodule.Let's extract this code in its own function (in
allauth_ens.utils?), then it becomes testable (and we all love that):allauthis magic! So cool…If we change as I propose above, it's only useful to avoid username clashes.
data.get('annee', '00')-> `data['annee'] ?No need to
.strip().lower()here ? Maybe we can add the case whereuidis in ldap infos to_extract_infos_from_ldap.Why the
namekey is always here, eventually blank, while the others are not here ?No idea, I think it only comes from the code I copy-pasted a long time ago to make LDAP work, I will test without.
So far, the problem is that
extra_datais overwritten with a blank dict at each connexion without a LDAP query, that's why I use the username. I thought about preventing that but iirc it would need some arguable hooks in the provider code, or deeper tweaks to the adapter...changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
changed this line in version 11 of the diff
added 1 commit
17fef409- More readable and organized codeCompare with previous version
added 1 commit
5ee1c774- Better README display?Compare with previous version
added 1 commit
4cf633ed- Actually no, too badCompare with previous version
added 1 commit
08a47150- Populate user model with promotion infosCompare with previous version
assigned to @delobell
added 1 commit
35a3bc3e- Small fix to example siteCompare with previous version
changed this line in version 16 of the diff
added 2 commits
534e0b18- LongTermClipper preserving LDAP data6e77b31e- Merge branch 'aureplop/archicubes_keep-ldap' into 'Evarin/archicubes'Compare with previous version
Your MR solved this discussion, I resolve it.
It works with actual LDAP, so it's resolved.
resolved all discussions
On se la tente ? @champeno
Allez j'y crois
Presque...
Et aussi :
Et plusieurs tests foirent avec Python 3.4 seulement. Toujours à cause de
_setup_ldap:Pour éviter de se faire toute la matrice de tox :
Je vais tenter d'ajouter la CI
added 2 commits
b189c114- Detoxify + debug py346a79b02f- Merge branch 'Evarin/archicubes' of…Compare with previous version
Damnit
J'ai pas pu tester en py34 (Error interpreter not found, et je peux pas apt-get install py34), mais ça devrait être résolu.
Il faudrait
bytes("..." % ..., encoding="utf-8")pour py3.Et s'en tenir à
b"..." % ...pour py2 (bytes prend pasencoding...).Tu dois avoir
import six; six.PY2 or six.PY3.Au passage, c'est le moment où pyenv peut se rendre utile.
added 1 commit
a812a43a- Compatibilite des tests en py2Compare with previous version
En fait deux lignes plus haut je gérais ça proprement pour le username, j'ai copié, ça marche en py27 et py35.
J'attends que tout tox tourne. Si c'est ok je ferme les yeux et je merge ;-)
resolved all discussions
merged
mentioned in commit
bc2b606288mentioned in merge request !2