Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2b65404
fix conftest to comply to exception implementation in sarc/ldap/super…
nurbal Feb 23, 2024
9433bcc
conftest exception for delegations and supervisors overrides
nurbal Feb 23, 2024
0b5763b
test for supervisors overrides
nurbal Feb 23, 2024
8b5d2b8
supervisors override implementation
nurbal Feb 23, 2024
9f0f418
add teacher_delegations in User API
nurbal Feb 23, 2024
c69a338
add test for teacher_delegations + lint
nurbal Feb 23, 2024
e7f3393
remove print
nurbal Feb 23, 2024
2418574
add teachers delegations implementation
nurbal Feb 23, 2024
abc367b
fix tests made without exceptions file
nurbal Feb 23, 2024
f206155
lint
nurbal Feb 23, 2024
10a720a
fix test_query_to_ldap_server_and_writing_to_output_json (ignore co_s…
nurbal Mar 1, 2024
8cef544
Merge commit 'be44d1a5f2ee0ed5bac764009e8d3ed87518cd2a' into users_ex…
nurbal Mar 1, 2024
9d61cb4
updated test_acquire_users_supervisors and test_acquire_users_co_supe…
nurbal Mar 1, 2024
7469517
lint/black/whatever
nurbal Mar 1, 2024
a95b9e7
Merge branch 'master' into users_exceptions
nurbal Apr 11, 2024
627f9d6
lint
nurbal Apr 11, 2024
9249978
Move the supervisors override and prof delegation exceptions to sarc.…
nurbal Apr 16, 2024
de015ae
Merge branch 'master' into users_exceptions
nurbal Apr 16, 2024
ad9f863
Merge branch 'master' into users_exceptions
nurbal Apr 18, 2024
56eb39c
fix test `test_load_job_series_with_users` to include the new `teache…
nurbal Apr 18, 2024
0329790
Merge branch 'master' into users_exceptions
nurbal Apr 23, 2024
8644834
Merge commit 'bab50a574ce25cf52ccf45fcd8aa4638f14da6c0' into users_ex…
nurbal Sep 21, 2024
a32536f
lint
nurbal Sep 25, 2024
ce1523d
fix tests: `test_get_user` and `test_get_users`
nurbal Sep 30, 2024
b945460
update lint exceptions ( disable too-many-positional-arguments )
nurbal Sep 30, 2024
59cab5f
fix test `test_acquire_ldap_revision_change`
nurbal Oct 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sarc/ldap/acquire.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import sarc.ldap.mymila
from sarc.config import config
from sarc.ldap.mymila import fetch_mymila
from sarc.ldap.read_mila_ldap import fetch_ldap
from sarc.ldap.read_mila_ldap import fetch_ldap, load_ldap_exceptions
from sarc.ldap.revision import commit_matches_to_database


Expand Down Expand Up @@ -92,7 +92,13 @@ def run(prompt=False):
# "drac_members": {...} or None
# }

exceptions = load_ldap_exceptions(cfg.ldap)
Copy link
Member

Choose a reason for hiding this comment

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

On essaie de partitionner chaque phase de chargement pour pouvoir les convertir en module avec une interface générique. Par exemple, le code de la ligne 35 https://github.com/mila-iqia/SARC/pull/103/files#diff-e9c775d5112b754dc796c041cfd42e5feddc9419366751fe3e0ffa2b9a025a73R35 aurait du se retoruver dans fetch_mymila.

Ces exceptions sont surtout en lien avec mymila, je crois que ça devrait aussi être dans fetch_mymila.

Copy link
Collaborator Author

@nurbal nurbal Apr 11, 2024

Choose a reason for hiding this comment

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

En l'occurrence, pas tout à fait. Pour les exceptions de superviseurs, OUI c'est en lien avec MyMila. Par contre pour les délégations de supervision, NON cette info n'est pas présente dans les 3 sources de données (DRAC, LDAP et MyMila) donc elles ont un scope "général".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ce que je veux dire, c'est que ces exceptions sont appliquées après le matching des 3 sources de données, indépendemment de la provenance des valeurs des champs concernés. Elles sont "agnostiques" si on veut...
Ça me paraissait donc logique de les appliquer à ce niveau là.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, donc dans un soucis de rendre ça le plus générique possible, ça serait comme un module supplémentaire dans le pipeline qui ajoute des corrections aux sources précédentes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fxed in 9249978


for _, user in DD_persons_matched.items():
if user["mila_ldap"]["mila_email_username"] in exceptions["delegations"]:
user["teacher_delegations"] = exceptions["delegations"][
user["mila_ldap"]["mila_email_username"]
]
fill_computed_fields(user)

# These associations can now be propagated to the database.
Expand Down
2 changes: 2 additions & 0 deletions sarc/ldap/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class User(BaseModel):
mila: Credentials
drac: Optional[Credentials]

teacher_delegations: Optional[list[str]] = None

mila_ldap: dict
drac_members: Optional[dict]
drac_roles: Optional[dict]
Expand Down
1 change: 1 addition & 0 deletions sarc/ldap/revision.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ def user_insert(newuser: dict) -> list:
"mila",
"drac_roles",
"drac_members",
"teacher_delegations",
)

update = {
Expand Down
18 changes: 17 additions & 1 deletion sarc/ldap/supervisor.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def sortkey(x):
return sorted(supervisors, key=sortkey, reverse=True)


# pylint: disable=too-many-branches
def resolve_supervisors(
ldap_people: list[dict], group_to_prof: dict, exceptions: dict
) -> SupervisorMatchingErrors:
Expand Down Expand Up @@ -204,7 +205,22 @@ def resolve_supervisors(
people.append(result)

for person in people:
if person.is_student:
# if there is a supervisors override, use it whatever the student status may be
if exceptions and person.ldap["mail"][0] in exceptions.get(
"supervisors_overrides", []
):
supervisors = exceptions["supervisors_overrides"][person.ldap["mail"][0]]
person.ldap["supervisor"] = None
person.ldap["co_supervisor"] = None
if len(supervisors) >= 1:
person.ldap["supervisor"] = supervisors[0]
else:
person.ldap["supervisor"] = None
if len(supervisors) >= 2:
person.ldap["co_supervisor"] = supervisors[1]
else:
person.ldap["co_supervisor"] = None
elif person.is_student:
person.ldap["is_student"] = True

supervisors = _extract_supervisors_from_groups(
Expand Down
20 changes: 18 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,24 @@ def file_contents():
"""
exceptions_json_path = """
{
"not_prof": [],
"not_student": []
"not_teacher": [],
"not_student": [],
"delegations": {
"[email protected]": [
"[email protected]",
"[email protected]"
]
},
"supervisors_overrides": {
"[email protected]": [
"[email protected]"
],
"[email protected]": [
"[email protected]",
"[email protected]"
]
}

}
"""

Expand Down
55 changes: 47 additions & 8 deletions tests/functional/cli/acquire/test_acquire_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,37 @@ def mock_query_ldap(
js_user = get_user(drac_account_username="stranger.person")
assert js_user is None

# test supervisor overrides
js_user = get_user(mila_email_username="[email protected]")
assert js_user is not None
assert js_user.mila_ldap["supervisor"] == "[email protected]"
assert js_user.mila_ldap["co_supervisor"] == None

js_user = get_user(mila_email_username="[email protected]")
assert js_user is not None
assert js_user.mila_ldap["supervisor"] == "[email protected]"
assert js_user.mila_ldap["co_supervisor"] == "[email protected]"

# test delegations
# john.smith003 should have delegations for john.smith004 and john.smith005
# john.smith004 should have no delegations
# john.smith005 should have no delegations

js_user = get_user(mila_email_username="[email protected]")
assert js_user is not None
assert js_user.teacher_delegations is not None
assert "[email protected]" in js_user.teacher_delegations
assert "[email protected]" in js_user.teacher_delegations
assert "[email protected]" not in js_user.teacher_delegations

js_user = get_user(mila_email_username="[email protected]")
assert js_user is not None
assert js_user.teacher_delegations == None

js_user = get_user(mila_email_username="[email protected]")
assert js_user is not None
assert js_user.teacher_delegations == None


@pytest.mark.parametrize(
"ldap_supervisor,mymila_supervisor,expected_supervisor",
Expand Down Expand Up @@ -163,6 +194,10 @@ def test_acquire_users_supervisors(
nbr_users = 4
nbr_profs = 2

# for the test we will use the user with index 3,
# which is the first user who has no supervisor override in the mock data
# so that this test won't be affected by the previous test

# Mock the fake LDAP data used for the tests
def mock_query_ldap(
local_private_key_file, local_certificate_file, ldap_service_uri
Expand All @@ -171,7 +206,7 @@ def mock_query_ldap(
return fake_raw_ldap_data(
nbr_users,
hardcoded_values_by_user={
2: { # The first user who is not a prof is the one with index 2
3: { # The first user who is not a prof is the one with index 3
"supervisor": ldap_supervisor
}
},
Expand All @@ -185,7 +220,7 @@ def mock_query_mymila(tmp_json_path):
nbr_users=nbr_users,
nbr_profs=nbr_profs,
hardcoded_values_by_user={
2: { # The first user who is not a prof is the one with index 2
3: { # The first user who is not a prof is the one with index 3
"Supervisor Principal": mymila_supervisor
}
},
Expand All @@ -208,8 +243,8 @@ def mock_query_mymila(tmp_json_path):

# Validate the results of all of this by inspecting the database.
js_user = get_user(
mila_email_username=f"john.smith002@mila.quebec"
) # We modified the user with index 2; thus this is the one we retrieve
mila_email_username=f"john.smith003@mila.quebec"
) # We modified the user with index 3; thus this is the one we retrieve
assert js_user.mila_ldap["supervisor"] == expected_supervisor


Expand Down Expand Up @@ -255,6 +290,10 @@ def test_acquire_users_co_supervisors(
nbr_users = 4
nbr_profs = 2

# for the test we will use the user with index 3,
# which is the first user who has no supervisor override in the mock data
# so that this test won't be affected by the previous test

# Mock the fake LDAP data used for the tests
def mock_query_ldap(
local_private_key_file, local_certificate_file, ldap_service_uri
Expand All @@ -263,7 +302,7 @@ def mock_query_ldap(
return fake_raw_ldap_data(
nbr_users,
hardcoded_values_by_user={
2: { # The first user who is not a prof is the one with index 2
3: { # The first user who is not a prof is the one with index 3
"co_supervisor": ldap_co_supervisor
}
},
Expand All @@ -277,7 +316,7 @@ def mock_query_mymila(tmp_json_path):
nbr_users=nbr_users,
nbr_profs=nbr_profs,
hardcoded_values_by_user={
2: { # The first user who is not a prof is the one with index 2
3: { # The first user who is not a prof is the one with index 3
"Co-Supervisor": mymila_co_supervisor
}
},
Expand All @@ -300,8 +339,8 @@ def mock_query_mymila(tmp_json_path):

# Validate the results of all of this by inspecting the database.
js_user = get_user(
mila_email_username=f"john.smith002@mila.quebec"
) # We modified the user with index 2; thus this is the one we retrieve
mila_email_username=f"john.smith003@mila.quebec"
) # We modified the user with index 3; thus this is the one we retrieve
assert js_user.mila_ldap["co_supervisor"] == expected_co_supervisor


Expand Down
3 changes: 0 additions & 3 deletions tests/functional/ldap/test_functional_revision.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ def user_with_history(username):

@pytest.mark.usefixtures("write_setup")
def test_get_user_fetch_latest():

user_with_history("user1")

#
Expand Down Expand Up @@ -140,7 +139,6 @@ def test_update_status_nodb_snapshots(status):
@pytest.mark.usefixtures("write_setup")
@pytest.mark.parametrize("status", statuses)
def test_update_status_db_nosnapshots(status):

collection = users_collection().get_collection()
update_user(collection, make_user(status))
users = user_dict([])
Expand Down Expand Up @@ -171,7 +169,6 @@ def test_update_status_db_nosnapshots(status):
@pytest.mark.usefixtures("write_setup")
@pytest.mark.parametrize("start,end", transitions())
def test_update_status_users_exists_on_both(start, end):

collection = users_collection().get_collection()
update_user(collection, make_user(start, record_start=datetime(2000, 1, 1)))
documents_before = list(collection.find({}))
Expand Down
1 change: 1 addition & 0 deletions tests/functional/ldap/test_read_mila_ldap.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def mock_query_ldap(

# resolve_supervisors is not called here
e["supervisor"] = None
e["co_supervisor"] = None

assert e == processed_user

Expand Down
1 change: 0 additions & 1 deletion tests/unittests/ldap/test_backfill.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ def userhistory(email, history):
latest_record = base_user

for start, fields in history:

if prev_fields is not None:
prev_fields["End Date with MILA"] = start
latest_record = dictset(latest_record, prev_fields)
Expand Down
1 change: 0 additions & 1 deletion tests/unittests/ldap/test_revision.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ def test_ldap_update_status_db_noldap(status):

@pytest.mark.parametrize("start,end", transitions())
def test_ldap_update_status_users_exists_on_both(start, end):

collection = MockCollection(
[STANDARD_USER, make_user(start, record_start=datetime(2000, 1, 1))]
)
Expand Down