Bug 53744 - Enable mapped udm properties for all Kelvin Resources
Summary: Enable mapped udm properties for all Kelvin Resources
Status: CLOSED FIXED
Alias: None
Product: UCS@school
Classification: Unclassified
Component: HTTP-API (Kelvin)
Version: UCS@school 4.4
Hardware: Other Mac OS X 10.1
: P5 normal
Target Milestone: ---
Assignee: Ole Schwiegert
QA Contact: Tobias Wenzel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-06 07:29 CEST by Ole Schwiegert
Modified: 2021-09-10 14:23 CEST (History)
2 users (show)

See Also:
What kind of report is it?: Feature Request
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Customer ID:
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ole Schwiegert univentionstaff 2021-09-06 07:29:57 CEST
We want to introduce the feature for mapped_udm_properties, which is present in the user resource, to all resources in the UCS@school Kelvin API.
Comment 1 Ole Schwiegert univentionstaff 2021-09-08 14:04:45 CEST
We migrated the mapped_udm_properties functionality from the ImportUser to the ucsschool.lib in general -- for all object types. This change was made in Kelvin only and shall not be ported to UCS@school 4.4 or 5.0 (as discussed in the team)

To configure which udm properties are available for which resource you have to edit the file /etc/ucsschool/kelvin/mapped_udm_properties.json as described in the added documentation. Then restart the app.

The following aspects should be considered in the QA:

- Only configured UDM properties can be read and written
- Only udm properties that are not directly linked to an already existing model field can be configured ans mapped udm properties.
- The configuration file does not have to exist. If it is missing the app runs and treats the configuration as empty lists for all values
- The configuration via import config (kelvin.json) still works
- If for the user resource both, the import config and the new config exist, the new config file has precedence. There is no merge, but complete override!
- If the configuration is invalid (incorrect json) the app fails early and logs the traceback
- If not configured udm properties are send in a POST/PUT/CREATE a 422 is the return
- The new field udm_properties is optional in all resources (especially PUT)!

Tests were added, documentation extended. The version 1.5.0 was created in the TestAppCenter and contains the newest code at time of writing.

Merge Request containing the changes for Kelvin: https://git.knut.univention.de/univention/ucsschool/-/merge_requests/65

ucs-test-ucsschool was changed, because of the new volume.

Package: ucs-test-ucsschool
Version: 6.0.246A~4.4.0.202109081402
Branch: ucs_4.4-0
Scope: ucs-school-4.4
Comment 2 Tobias Wenzel univentionstaff 2021-09-09 16:30:00 CEST
QA: Reopen (majority works, but some remarks)

- The content of the udm-propeties is not logged (only when the api is started from within the container), this is different than before. Add this if possible.
- As discussed, there need to be some more tests.


- [x] code review with implementee
- [x] code review
When using the swagger-ui we get the doc string of the path. For users we also have "udm_properties: object with UDM properties (optional, e.g. {"street": "Luise Av."}, must be configured in kelvin.json in mapped_udm_properties, see documentation)"
-> This is missing for school_classes and schools

- [x] kelvin-api tests pass in docker container
- [x] build docker container locally
- [x] install new version in multi-server env
- [x] update with old kelvin + config
- [x] changelogs
- [x] version 1.5.0


functional test in multi-server env (primary)

UCS: 4.4-8 errata1044
Installed: ucsschool=4.4 v9 ucsschool-kelvin-rest-api=1.5.0

- [x] Only configured UDM properties can be read and written
- [ ] Only udm properties that are not directly linked to an already 
existing model field can be configured as mapped udm properties.

REOPEN -> this should not be possible, right? (in user.get)

    "udm_properties": {
      "description": null,
      "name": "DEMOSCHOOL"
    },
    "name": "DEMOSCHOOL"
    
-> i got the expected error 422 when using put+post+patch


- [x] The configuration file does not have to exist. If it is missing the app runs and treats the configuration as empty lists for all values
- [x] The configuration via import config (kelvin.json) still works
- [x] If for the user resource both, the import config and the new config exist, the new config file has precedence. There is no merge, but complete override!
- [x] If the configuration is invalid (incorrect json) the app fails early and logs the traceback
- [ ] If not configured udm properties are send in a POST/PUT/PATCH a 422 is the return

When doing a user.patch with an udm_property which was not inside the config, I did not get an error (the value was ignored)

{
"udm_properties": {
    "country": "England"
  }
}
user post, put -> 422


- [x] The new field udm_properties is optional in all resources (especially PUT)!
  - school-classes post,put
  - user post put


- [ ] proof reading of documentation

REOPEN, some suggestions:

There was already an ``udm_properties`` functionality available for user resources  within Kelvin. With the release of Kelvin 1.5.0 the ``udm_properties`` functionality was added to all other resources (except roles) as well. 

[...]

The format of the ``mapped_udm_properties.json`` is:
``{name_of_resource:["name_of_property_to_map",...],...}``   

[...]

The Kelvin configuration contains also a ``mapped_udm_properties``. This referes to the user resource.  If there is also a configuration for the key ``user`` in ``mapped_udm_properties.json``, it will override the ``mapped_udm_propertes`` kelvin configuration (for users only).
Comment 3 Ole Schwiegert univentionstaff 2021-09-09 23:28:18 CEST
- [ ] Only udm properties that are not directly linked to an already 
existing model field can be configured as mapped udm properties.

Actually an important questions. For writing access this is implemented, as you noted before. But it is indeed possible right now to configure said fields as mapped. This leads to the behavior that they are returned for reading, but cannot be written.

This behavior might be desired? I will make a note in tomorrows daily.

If accepted behavior this needs to be explicitly stated in the documentation

If the behavior is undesired we have to introduce a mechanism that will detect those faulty properties already on startup time of the app. That might be a bit dicey, we have to see.


- [ ] If not configured udm properties are send in a POST/PUT/PATCH a 422 is the return

Even worse in PATCH any udm properties in school_class was allowed and saved! Thanks for the find. Fixed

- [ ] proof reading of documentation

Best to incorporate it swiftly together tomorrow!

I rebased to the new feature/kelvin state so a force push was necessary. Also the image was rebuild for 1.5.0


Tests get expanded tomorrow morning.
Comment 4 Tobias Wenzel univentionstaff 2021-09-10 11:06:39 CEST
Documentation suggestions have been added in:

[oschwieg/53744] bf2e635ee Bug #53744: add documentation

OK new docker container 
OK tested post/put/patch for user + class 

I copied the new tests into my docker container and they passed.

[oschwieg/53744] f30da3691 Bug #53744: Add pyfakefs to test requirements
[oschwieg/53744] ba3a64896 Bug #53744: Add tests for config
[oschwieg/53744] 9682c675e Bug #53744: Add tests for unmapped properties
Comment 5 Ole Schwiegert univentionstaff 2021-09-10 12:59:59 CEST
Little addition:

#. Any udm property that is directly linked to an already existing model field results in an invalid configuration.
   It is not allowed, for example, to configure the ``description`` of a school class as an udm property, since it is
   already present in the model itself. This is now also true for the user resource, where this was possible before.

We now fail early and the app will not start, if this rule is violated.
Comment 6 Tobias Wenzel univentionstaff 2021-09-10 14:23:23 CEST
QA: Verified

- new installation from production app center 1.5.0
- upgrade from production app center 1.4.4 -> 1.5.0
- docs.univention.de
https://docs.software-univention.de/ucsschool-kelvin-rest-api/installation-configuration.html#udm-properties
- mail notification with working link to changelog https://appcenter.software-univention.de/univention-repository/4.4/maintained/component/ucsschool-kelvin-rest-api_20210902124852/README_UPDATE_DE
Comment 7 Ole Schwiegert univentionstaff 2021-09-10 14:23:49 CEST
Kelvin REST API 1.5.0 has been released.

https://appcenter.software-univention.de/univention-repository/4.4/maintained/component/ucsschool-kelvin-rest-api_20210902124852/README_UPDATE_DE

If this error occurs again, please clone this bug.