Bug 42416 - [4.3] The user role should be configurable in the CSV file
[4.3] The user role should be configurable in the CSV file
Status: CLOSED FIXED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 4.1 R2
Other Linux
: P5 normal (vote)
: UCS@school 4.3 v4
Assigned To: Daniel Tröder
Ole Schwiegert
:
Depends on:
Blocks: 47073
  Show dependency treegraph
 
Reported: 2016-09-15 16:34 CEST by Florian Best
Modified: 2018-07-04 18:09 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 2: Improvement: Would be a product improvement
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.046
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): Cleanup
Max CVSS v3 score:


Attachments
log with exception (6.41 KB, text/x-log)
2018-06-26 10:06 CEST, Ole Schwiegert
Details
the csv used to produce error (297 bytes, text/csv)
2018-06-26 10:06 CEST, Ole Schwiegert
Details
The mapping used during test (802 bytes, application/json)
2018-06-26 10:07 CEST, Ole Schwiegert
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2016-09-15 16:34:36 CEST
Currently the user role can only be defined in the CSV file if using the:
ucsschool.importer.reader.test_csv_reader.TestCsvReader

With a mapping of to a private property:
"Benutzertyp": "__type",

This should be easier!
Comment 1 Daniel Tröder univentionstaff 2016-09-16 08:21:29 CEST
I also vote for merging TestCsvReader into CsvReader in the product to officially support "__type" as part of "Sonderwerte" (http://docs.software-univention.de/ucsschool-import-handbuch-4.1R2.html#configuration:mapping), currently: "__action" and "__ignore".

IMHO it is very likely that some customer will need this. With a distributed CSV-Import-module (-> more diverse input-sources) this becomes even more likely.
Comment 2 Daniel Tröder univentionstaff 2017-03-10 12:19:49 CET
Please also write a ucs-test that tests with input
* that is missing the column
* for each of the 4 user types
* with invalid input
Comment 3 Jürn Brodersen univentionstaff 2017-04-05 17:28:16 CEST
The manual recommends not setting --user-role during an import if the role is set in the import data. Which doesn't really work, see above.

http://docs.software-univention.de/ucsschool-import-handbuch-4.1R2.html#table:userimport_configuration
Comment 4 Daniel Tröder univentionstaff 2017-04-06 08:43:19 CEST
(In reply to Jürn Brodersen from comment #3)
> The manual recommends not setting --user-role during an import if the role
> is set in the import data. Which doesn't really work, see above.
> 
> http://docs.software-univention.de/ucsschool-import-handbuch-4.1R2.
> html#table:userimport_configuration
I don't understand what isn't working, please clarify.

But I see the gap in the documentation:
It is missing a definition what happens if both is set: user-type in CSV _and_ --user-role.
Comment 5 Jürn Brodersen univentionstaff 2017-04-06 10:03:55 CEST
(In reply to Daniel Tröder from comment #4)
> (In reply to Jürn Brodersen from comment #3)
> > The manual recommends not setting --user-role during an import if the role
> > is set in the import data. Which doesn't really work, see above.
> > 
> > http://docs.software-univention.de/ucsschool-import-handbuch-4.1R2.
> > html#table:userimport_configuration
> I don't understand what isn't working, please clarify.
> 
> But I see the gap in the documentation:
> It is missing a definition what happens if both is set: user-type in CSV
> _and_ --user-role.

Sorry, I meant that you can't set the user-type in CSV.
E.g. the following mapping doesn't work:
'''
    "csv": {
        "mapping": {
            "Schulen": "schools",
            "Vorname": "firstname",
            "Nachname": "lastname",
            "Klassen": "school_classes",
            "Mailadresse": "email",
            "Telefon": "phone",
            "Beschreibung": "description",
            "Benutzertyp": "user_role"
        } 
    }
'''
If no --user-role is set:
"ERROR cmdline.main:124  2: No role in configuration."
and with --user-role set:
"ERROR cmdline.main:124  2: Unknown UDM property: 'user_role'."

The "__type" mapping in comment 1 doesn't seem to work with "ucs-school-user-import" neither.
Comment 6 Daniel Tröder univentionstaff 2018-05-24 20:13:30 CEST
New user role assignment can now be handled by the regular CSV reader class CsvReader. This allows the use of the special mapping value "__role".
Allowed values in the CSV column are student, staff, teacher and teacher_and_staff.
The use of the TestCsvReader class is no longer required. For backwards compatibility the class still exists and allows to use "__type".

[4.3] a3d96076 Bug #42416: move handling of special mapping value for user role to regular CsvReader
[4.3] 3ce6de59 Bug #42416: changelog
[4.3] 6bf235c0 Bug #42416: adapt tests to use the new special mapping value in the regular CsvReader
[4.3] ecdc15fa Bug #42416: changelog
[4.3] b1ae0b2c Bug #42416: advisory

ucs-school-import (16.0.1-29)
ucs-test-ucsschool (5.0.2-50)
Comment 8 Ole Schwiegert univentionstaff 2018-06-26 10:06:14 CEST
Created attachment 9574 [details]
log with exception

changelog OK
Advisory OK
Documentation OK
Import with __role mapping OK
Import wit __role and --user_role REOP

When you import a csv with a __role mapping but also use the --user_role option the import crashes with a noObject exception if there is at least one row in the CSV that does not have the role specified in the command line parameter.

This is because the created object got the role specified in the CSV but was expected to have the role specified in --user_role. The documentation states that the command line parameter should not be used if a __role attribute is present but the import should not just crash.

In my opinion there should also be a clear rule for what option gets priority.

I added the csv and the resulting log as attachements to the bug. The command used was:

/usr/share/ucs-school-import/scripts/ucs-school-user-import --conffile /var/lib/ucs-school-import/configs/user_import.json --infile test.csv --school AE
Comment 9 Ole Schwiegert univentionstaff 2018-06-26 10:06:41 CEST
Created attachment 9575 [details]
the csv used to produce error
Comment 10 Ole Schwiegert univentionstaff 2018-06-26 10:07:53 CEST
Created attachment 9576 [details]
The mapping used during test
Comment 11 Ole Schwiegert univentionstaff 2018-06-26 10:32:04 CEST
I forgot to add a remark about the documentation: In line 984 of ucsschool-import-handbuch-4.3.xml:

Der Inhalt dieser Spalte wird ignoriert. Die kann z.B. verwendet werden, wenn die CSV-Datei leerer Spalten, oder solche mit nicht zu importierenden Daten, enthält.

What does 'Die' refer to. The Sonderwerte from the beginning of the chapter?
I guess it should be 'leere Spalten'

This sentence is a bit hard to grasp in my opinion because of the "Die" and since it was already edited here it might be fixed here as well.
Comment 12 Daniel Tröder univentionstaff 2018-06-28 10:30:05 CEST
(In reply to Ole Schwiegert from comment #8)
> Import wit __role and --user_role REOP
> 
> When you import a csv with a __role mapping but also use the --user_role
> option the import crashes with a noObject exception if there is at least one
> row in the CSV that does not have the role specified in the command line
> parameter.
> 
> This is because the created object got the role specified in the CSV but was
> expected to have the role specified in --user_role. The documentation states
> that the command line parameter should not be used if a __role attribute is
> present but the import should not just crash.
> 
> In my opinion there should also be a clear rule for what option gets
> priority.

The documentation clearly states, that such a combination is not allowed:
---------------------------------------------------------
user_role / -u / --user_role:
Definiert die Benutzerrolle für alle Eingabedatensätze. Diese Variable sollte nur gesetzt werden, wenn die Benutzerrolle nicht in den Eingabedaten enthalten ist und die Eingabedatensätze homogen alle die gleiche Benutzerrolle verwenden sollen.
---------------------------------------------------------

I have now additionally documented it in the section about "__role" and the import now also supports "none" on the cmdline for --user_role (to be able to overwrite a configuration file setting).

It is now checked if both "user_role" and "__role" in config['csv']['mapping'] is used at the same time, and an InitialisationError is raised to prevent an import run with that combination. Prioritization is not allowed, as this is a contradictory configuration is most likely a user error.

[4.3] c742d8c83 Bug #42416: prevent running forbidden configuration combination
[4.3] 3e5aac541 Bug #42416: changelog
[4.3] fbdde11fa Bug #42416: advisory update

ucs-school-import (16.0.2-18)
Comment 13 Ole Schwiegert univentionstaff 2018-06-28 11:57:25 CEST
changelog OK
Advisory OK
Documentation OK
Import with __role mapping OK
Import wit __role and --user_role OK
Comment 14 Sönke Schwardt-Krummrich univentionstaff 2018-07-04 18:09:03 CEST
UCS@school 4.3 v4 has been released.

https://docs.software-univention.de/changelog-ucsschool-4.3v4-de.html

If this error occurs again, please clone this bug.