Univention Bugzilla – Bug 56340
SiSoPi Import removes class memberships in other schools
Last modified: 2023-09-12 13:41:53 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?
## 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: ```bash /usr/share/ucs-school-import/scripts/create_ou School1 /usr/share/ucs-school-import/scripts/create_ou School2 ``` Then create the following import files: `school_1.csv`: ```csv "Schule","Vorname","Nachname","Klassen","Beschreibung","Telefon","EMail" "School1","Vinny","Foo","1a","A teacher.","+67-303-103581","" ``` `school_2.csv`: ```csv "Schule","Vorname","Nachname","Klassen","Beschreibung","Telefon","EMail" "School2","Vinny","Foo","2a","A teacher.","+67-303-103581","" ``` Run the import for the first school: ```bash /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: ```bash /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`: ```python new_classes = copy.deepcopy(old_user.school_classes) new_classes.update(imported_user.school_classes) imported_user.school_classes = new_classes ```
Created attachment 11098 [details] bug_56340.patch
## 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: ```python >>> 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 '/tmp/34_import-users_via_cli_v2.wkbqep57/users.9u0wtsm0' ``` The corresponding output file looks like the following: ```text "Gruppen","Nach","Vor","OUs","Beschreibung","E-Mail" "1a,1b,2a,2b","wi3sq4warc","uhlslv3ksh","School1,School2","","ob12ybipve@demoschool.example.com" ``` But it is supposed to look like: ```text "Gruppen","Nach","Vor","OUs","Beschreibung","E-Mail" "School1-1a,School1-1b,School2-2a,School2-2b","wi3sq4warc","uhlslv3ksh","School1,School2","","ob12ybipve@demoschool.example.com" ``` 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: ```text "Gruppen","Nach","Vor","OUs","Beschreibung","E-Mail" "1a,1b","wi3sq4warc","uhlslv3ksh","School1","","ob12ybipve@demoschool.example.com" ``` ```text "Gruppen","Nach","Vor","OUs","Beschreibung","E-Mail" "2a,2b","wi3sq4warc","uhlslv3ksh","School2","","ob12ybipve@demoschool.example.com" ``` The SiSoPi test's `create_csv_file` will have to create one file for each school.
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.
I have implemented the fix and currently work on adapting the tests.
Issue has been fixed with Commit 65880420 in ucsschool.
Issue fixed with: Package: ucs-school-import Version: 18.0.33
Everything looks fine on the issue ;)
Errata updates for UCS@school 5.0 v4 have been released. https://docs.software-univention.de/ucsschool-changelog/5.0v4/en/changelog.html https://docs.software-univention.de/ucsschool-changelog/5.0v4/de/changelog.html If this error occurs again, please clone this bug.