ORGU / MD: 42725, delete OrgUnit type assignments if ...

[lukastocker]

... any advanced meta data set is assigned and deleted without de-assigning the set beforehand.

Remove assigned meta data sets (by OrgUnit > Types) which were deleted but not de-assigned via a setup objective achieve process.

https://mantis.ilias.de/view.php?id=42725

Please note that you have to run 'php cli/setup.php achieve orgunit.removeDeletedMDSetsFromOrgUnitTypes'.

[lukastocker]

Hi @lukastocker,

do I understand correctly that ilOrgUnitRemoveDeletedMDSetsObjective is supposed to clean up assignments that haven't been rectified till now, but that the code changes also mean that in the future all assignments will be cleaned up correctly?

If so, the objective should be a migration.

Kind regards!

You're absolutely correct. I will change this to a migration! Thanks for the hint! :)

[klees]

Hi @smeyer-ilias and @schmitz-ilias,

this contains a small addition in AdvancedMetadata to clean up affairs in the OrgUnits when Metadatasets are deleted...

If possible I would prefer to couple AMD and OrgUnits less closely, but we couldn't find another solution. If there's anything we shall do differently here we are of course available.

Kind regards!

[schmitz-ilias]

Hi @klees and @lukastocker,

I'm very much in favor of this clean up operation, but I'm a bit uneasy about the addition to ilAdvancedMDRecord. That class is fairly central to AdvMD, and instantiated quite a lot (probably too much). Introducing a dependency to a different component there, especially in the constructor, seems like asking for trouble. The fact that you had to make changes to unrelated unit tests sort of illustrates this.

Additionally, I don't think the issue you are solving is limited to OrgUnits, at least StudyProgramme also uses this 'types'-pattern, right? I'd prefer a solution that is a bit more scalable.

How about event handling? AdvMD could throw a 'recordDeleted' Event in ilAdvancedMDRecord and maybe also ilAdvancedMDClaimingPlugin, and other components could react to that. Would that work for you?

[klees]

Hi @schmitz-ilias,

yes, all this =)

So shall we simply add that event in this PR? Would you want to do it? Any more hints regarding the implementation?

Kind regards!

[schmitz-ilias]

Feel free to go ahead! As mentioned above, records can also be deleted via ilAdvancedMDClaimingPlugin, but please leave that alone for now. It desperately needs refactoring anyways, and deletion of records via plugin is enough of an edge case to be disregarded for now.

Let me know if you have any questions!

[klees]

Hi @schmitz-ilias,

could you have another look here?

Kind regards!

[schmitz-ilias]

Hi @lukastocker, @klees,

changes in AdvMD look good! Just one thing: please add the event to the service.xml of AdvMD, otherwise it won't be processed by event handling. It looks like the complementary event entry is also missing from the module.xml of OrgUnit.

Just a small thing, so no need for a re-review from my side. Thanks for the improvement!

Cheers, Tim

PS: I think there is also something not quite right with the changes on OrgUnit side to ilOrgUnitAppEventListener. Calling ilObject2::_lookupType with an AdvMD record ID doesn't work, and ilOrgUnitType::deleteOrgUnitTypeAssignmentByRecId is not static.

[lukastocker]

Hi @schmitz-ilias , @klees

the event is now listed in the xml files and the type is looked up correctly. Deleting a custom meta data set without withdrawing the assignment first will now delete the specific entry in the data table orgu_types_adv_md_rec.

[schmitz-ilias]

Hi @lukastocker, I noticed two things:

Checking for type before doing the clean-up seems like a good idea, but ilAdvancedMDRecord::_lookupTypeById will not work like this. This is not a one-to-one relationship, multiple different types can be assigned to the same record. If you want to keep the type check, you can instead pass the array of assigned object types as a parameter in the event, similar to 'record_id'. They are available in ilAdvancedMDRecord anyways via ilAdvancedMDRecord::getAssignedObjectTypes, so no need to read them out again. This would spare us from another unnecessary static method.

Also, the event entry in the service.xml of AdvMD is not quite right. AdvMD doesn't listen to the event, but raise it, so it should be type="raise" and the id should be the id of the event. See e.g. the module.xml of Course for examples.

Let me know if you have any further questions!

Cheers, Tim

[lukastocker]

Hi @schmitz-ilias

I just updated the PR according to your feedback.

Have a nice week!

[schmitz-ilias]

Thanks @lukastocker, looks good to me!