Bug 48045 - Classes are not removed if none are set in the infile
Classes are not removed if none are set in the infile
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 4.3
Other Linux
: P5 normal (vote)
: UCS@school 4.4 v1
Assigned To: Daniel Tröder
Ole Schwiegert
:
Depends on:
Blocks: 48479
  Show dependency treegraph
 
Reported: 2018-10-23 12:03 CEST by Jürn Brodersen
Modified: 2019-03-30 08:41 CET (History)
5 users (show)

See Also:
What kind of report is it?: Development Internal
What type of bug is this?: 4: Minor Usability: Impairs usability in secondary scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.091
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jürn Brodersen univentionstaff 2018-10-23 12:03:13 CEST
Classes are not removed if none are set in the infile

If a teacher is imported without classes (empty string) any previously set classes are not removed by the importer.

Changing classes removes the old class.

See bug 48028 for more information.


I think we should make the behavior consistent with other imported fields. UCS 4.4 might be a good time to change this.
Maybe a ucr variable should be set for old installation to not change that behavior?
Comment 1 Daniel Tröder univentionstaff 2019-01-07 13:20:35 CET
Change docu (see https://forge.univention.org/bugzilla/show_bug.cgi?id=48028)
Comment 2 Daniel Tröder univentionstaff 2019-01-17 11:28:55 CET
The ucsschool import and the underlying ucsschool.lib now remove a user from its school classes, if the school_classes attribute is empty.

This is an API breaking change!

The ucsschool import (not the lib!) supports a backwards compatibility switch in the JSON configuration:
---------------------------
{
    "school_classes_keep_if_empty": true
}
---------------------------
The legacy import that is based upon the import framework has this variable set to true.

The ucs-test 214_import-users_empty_class_column was extended to test both settings (school_classes_keep_if_empty=False is the default).

[4.4] 944030e75 Bug #48045: Bug #48045: remove classes if none are set in the infile / school_classes attribute
[4.4] da3ad2464 Bug #48045: Bug #48045: remove classes if none are set in the school_classes attribute
[4.4] 32df3c257 Bug #48045: Bug #48045: make ucslint happy
[4.4] 0057ca5ea Bug #48045: test new behavior of classes removal
[4.4] 967035985 Bug #48045: advisories

Documentation commit with change was labeled with wrong bug number:

[4.4] 44c53983a Bug #48045: document changed behavior

ucs-school-lib (12.1.0-1)
ucs-school-import (17.2.0-1)

(Minor versions raised because of API change.)
Comment 3 Jürn Brodersen univentionstaff 2019-02-11 15:05:44 CET
Test 34_import-users_via_cli fails:

[2019-02-11 14:37:52.129193] Traceback (most recent call last):
[2019-02-11 14:37:52.129222]   File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/mass_import.py", line 110, in import_users
[2019-02-11 14:37:52.129250]     user_import.create_and_modify_users(imported_users)  # 90% - 100%
[2019-02-11 14:37:52.129277]   File "/usr/lib/pymodules/python2.7/ucsschool/importer/mass_import/user_import.py", line 203, in create_and_modify_users
[2019-02-11 14:37:52.129303]     success = user.create(lo=self.connection)
[2019-02-11 14:37:52.129331]   File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 332, in create
[2019-02-11 14:37:52.129357]     res = super(ImportUser, self).create(lo, validate)
[2019-02-11 14:37:52.129384]   File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 459, in create
[2019-02-11 14:37:52.129408]     success = self.create_without_hooks(lo, validate)
[2019-02-11 14:37:52.129440]   File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/base.py", line 472, in create_without_hooks
[2019-02-11 14:37:52.129466]     self.validate(lo)
[2019-02-11 14:37:52.129550]   File "/usr/lib/pymodules/python2.7/ucsschool/importer/legacy/legacy_import_user.py", line 85, in validate
[2019-02-11 14:37:52.129581]     super(LegacyImportUser, self).validate(lo, validate_unlikely_changes, check_username)
[2019-02-11 14:37:52.129606]   File "/usr/lib/pymodules/python2.7/ucsschool/importer/models/import_user.py", line 997, in validate
[2019-02-11 14:37:52.129629]     super(ImportUser, self).validate(lo, validate_unlikely_changes)
[2019-02-11 14:37:52.129654]   File "/usr/lib/pymodules/python2.7/ucsschool/lib/models/user.py", line 413, in validate
[2019-02-11 14:37:52.129679]     if school.lower() not in map(str.lower, self.schools):
[2019-02-11 14:37:52.129704] TypeError: descriptor 'lower' requires a 'str' object but received a 'unicode'
Comment 4 Daniel Tröder univentionstaff 2019-02-11 15:49:17 CET
[4.4] cfb56800d Bug #48045: use lower() of both str and unicode

ucs-school-lib (12.1.0-3)
Comment 5 Ole Schwiegert univentionstaff 2019-02-20 10:58:40 CET
Changelog & Advisory: OK
Packages install: OK
Tests pass on Jenkins: OK
Classes are removed if import field is empty (empty string, empty, etc): OK
keep classes flag works: OK
Classes are removed if field is cleared in lib: OK

Seems all as intended.

Remark: The package versions stated in this ticket are not valid anymore, since they were changed afterwards
Comment 6 Ole Schwiegert univentionstaff 2019-02-20 10:58:52 CET
Addition: Doc: OK
Comment 7 Ole Schwiegert univentionstaff 2019-02-20 13:39:53 CET
If no mapping for classes is set, all class information will be removed upon a new import.

This has to be avoided.
Comment 8 Daniel Tröder univentionstaff 2019-02-21 17:18:25 CET
Please read comment #2.
Comment 9 Jürn Brodersen univentionstaff 2019-02-21 23:06:21 CET
Now it isn't consistent again. Classes seem to be the only attribute that get removed if not mapped.
Comment 10 Daniel Tröder univentionstaff 2019-02-22 12:58:37 CET
The problem with the school_classes property is, that the ucsschool.lib sets its initial value to {} if empty. So we cannot easily distinguish from its current value if the input was empty or there was no input (no mapping). But luckily we can access the original input data.

The behavior is now:

* if there is a mapping for school_classes
  - if the CSV-data is non-empty → set new school_classes
  - if the CSV-data is empty
    - if config[school_classes_keep_if_empty] is True → keep previous school_classes
    - else → empty school_classes
* if there is no mapping for school_classes → keep previous school_classes


[4.4 9537b9d7a] Bug #48045: figure out why the school_class property is empty and handle accordingly

ucs-school-import 17.0.4-7A~4.4.0.201902221250
Comment 11 Sönke Schwardt-Krummrich univentionstaff 2019-02-22 15:30:34 CET
(In reply to Daniel Tröder from comment #10)
> The behavior is now:
> 
> * if there is a mapping for school_classes
>   - if the CSV-data is non-empty → set new school_classes
>   - if the CSV-data is empty
>     - if config[school_classes_keep_if_empty] is True → keep previous
> school_classes
>     - else → empty school_classes
> * if there is no mapping for school_classes → keep previous school_classes

Great!
Comment 12 Ole Schwiegert univentionstaff 2019-02-25 13:12:40 CET
Changelog & Advisory: OK
Package installs: OK
Behaves as defined in Comment #10: OK

Everything works now according to the defined behavior.
Comment 13 Sönke Schwardt-Krummrich univentionstaff 2019-03-12 10:59:08 CET
UCS@school 4.4 v1 has been released.

https://docs.software-univention.de/release-notes-ucsschool-4.4v1-de.html

If this error occurs again, please clone this bug.