Bug 56340 - SiSoPi Import removes class memberships in other schools
SiSoPi Import removes class memberships in other schools
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 5.0
Other Linux
: P5 normal (vote)
: UCS@school 5.0 v4-errata
Assigned To: Alexander Steffen
J Leadbetter
Depends on:
  Show dependency treegraph
Reported: 2023-07-19 12:09 CEST by Frank Greif
Modified: 2023-09-12 13:41 CEST (History)
7 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 4: A User would return the product
User Pain: 0.229
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:
kiok: Patch_Available+

bug_56340.patch (713 bytes, patch)
2023-07-20 15:44 CEST, J Leadbetter
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Greif univentionstaff 2023-07-19 12:09:16 CEST
ucs 5.0.4, @school 5.0v3, kelvin 1.8.9

In the SiSoPi scenario, when importing users who shall be member of classes at multiple schools, some group memberships get lost: each import into one school removes all class memberships of other schools. What remains is a user who only has the correct class membership at the school that was imported last. Furthermore subsequent access to this student object yields validation warnings like "is missing groups at..."

To my understanding SiSoPi import at one school should leave assignments to other schools fully intact -- this is what I deemed the purpose of SiSoPi?
Comment 1 J Leadbetter univentionstaff 2023-07-20 15:07:03 CEST
## Reproduction

We have been able to reproduce the bug. How to reproduce:

Use the example `user_import.json` from the [SiSoPi documentation](https://docs.software-univention.de/ucsschool-umc-user-import/5.0/de/single-source.html#beispielaufbau). Create the following schools:

/usr/share/ucs-school-import/scripts/create_ou School1
/usr/share/ucs-school-import/scripts/create_ou School2

Then create the following import files:


"School1","Vinny","Foo","1a","A teacher.","+67-303-103581",""


"School2","Vinny","Foo","2a","A teacher.","+67-303-103581",""

Run the import for the first school:

/usr/share/ucs-school-import/scripts/ucs-school-user-import \
    --verbose \
    --user_role teacher \
    --infile school_1.csv \
    --school School1

The user should have the correct groups:

* Domain Users School1
* lehrer-School1
* School1-1a

Then run the import for the second school:

/usr/share/ucs-school-import/scripts/ucs-school-user-import \
    --verbose \
    --user_role teacher \
    --infile school_2.csv \
    --school School2

Notice the groups of the user is now missing the School1 classes:

* Domain Users School1
* Domain Users School2
* lehrer-School1
* lehrer-School2
* School2-2a

## Fix

The issue is that when importing the user in SiSoPi, it only takes the classes from the import file when setting up classes. The fix is to make sure that the old classes for other schools is included when writing the user.

The bug is fixed by adding the following lines at line 153 of `/usr/lib/python3/dist-packages/ucsschool/importer/mass_import/sisopi_user_import.py`:

                new_classes = copy.deepcopy(old_user.school_classes)
                imported_user.school_classes = new_classes
Comment 2 J Leadbetter univentionstaff 2023-07-20 15:44:31 CEST
Created attachment 11098 [details]
Comment 3 J Leadbetter univentionstaff 2023-07-20 15:45:02 CEST
## Test Fixes

We currently have [tests for SiSoPi](https://git.knut.univention.de/univention/ucsschool/-/blob/5.0/ucs-test-ucsschool/90_ucsschool/239_import-users_sisopi.py), but the tests are incorrectly creating the CSV file, so the tests are passing incorrectly.

Here are the issues that need to be fixed:

1. When exporting the csv for the users and the user has multiple schools, it exports the schools incorrectly. Here's how to reproduce in python3:

>>> from univention.testing.ucsschool.importusers import Person
>>> from univention.testing.ucsschool.importusers_cli_v2 import CLI_Import_v2_Tester
>>> person = Person('School1', 'teacher')
>>> person.update(school='School1', schools=['School1', 'School2'], school_classes={'School1': ['1a', '1b'], 'School2': ['2a', '2b']})
>>> person.school_classes
{'School1': ['1a', '1b'], 'School2': ['2a', '2b']}
>>> tester = CLI_Import_v2_Tester()
>>> fn_csv = tester.create_csv_file(person_list=[person])
>>> fn_csv

The corresponding output file looks like the following:


But it is supposed to look like:


When there is only one school, it is fine to write the classes without the school name prefix, but when there are multiple schools, it needs to write the school name prefix. This affects any tests (not just SiSoPi) that use multiple schools to test the import. We need to update `CLI_Import_v2_Tester.create_csv_file` to write this correctly when there are multiple schools.

2. For SiSoPi, the test should be writing to two separate import files, not one. Using the example above, it should write the following two import files:



The SiSoPi test's `create_csv_file` will have to create one file for each school.
Comment 5 Frank Greif univentionstaff 2023-07-25 13:53:44 CEST
I can confirm that at least with my test data, the patch from https://forge.univention.org/bugzilla/show_bug.cgi?id=56340#c2 fixes the issue. After execution of a SiSoPi import into multiple schools, the expected group memberships are exactly as expected.

Please disregard my statement about validation errors with text 'is missing groups at ...'. These are still showing up in the log, but are apparently related to the interim incomplete state of objects. In the end, the results are correct, and the related diagnostic checks for group memberships pass successfully.
Comment 6 Alexander Steffen univentionstaff 2023-08-22 07:42:50 CEST
I have implemented the fix and currently work on adapting the tests.
Comment 7 Alexander Steffen univentionstaff 2023-09-05 06:57:06 CEST
Issue has been fixed with Commit 65880420 in ucsschool.
Comment 8 Alexander Steffen univentionstaff 2023-09-07 13:18:12 CEST
Issue fixed with:
Package: ucs-school-import
Version: 18.0.33
Comment 9 Tobias Wenzel univentionstaff 2023-09-12 13:17:46 CEST
Everything looks fine on the issue ;)
Comment 10 Tobias Wenzel univentionstaff 2023-09-12 13:41:53 CEST
Errata updates for UCS@school 5.0 v4 have been released.


If this error occurs again, please clone this bug.