Bug 51500 - ResultPyHook is executed even if supports_dry_run==False
ResultPyHook is executed even if supports_dry_run==False
Status: REOPENED
Product: UCS@school
Classification: Unclassified
Component: Import scripts
UCS@school 4.4
Other Linux
: P5 normal (vote)
: ---
Assigned To: Toni Röhmeyer
Daniel Tröder
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-16 11:21 CEST by Daniel Tröder
Modified: 2020-11-20 11:05 CET (History)
2 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 3: Simply Wrong: The implementation doesn't match the docu
Who will be affected by this bug?: 1: Will affect a very few installed domains
How will those affected feel about the bug?: 1: Nuisance – not a big deal but noticeable
User Pain: 0.017
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): Workaround is available
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Tröder univentionstaff 2020-06-16 11:21:20 CEST
ResultPyHook is a subclass of ImportPyHook and should thus not be executed when its class attribute "supports_dry_run" is "False".

Change the import code so that ResultPyHooks are only executed if supports_dry_run==True.

Please also check if there are more subclasses of ImportPyHook that ignore the setting. Maybe the problem can be fixed for all together, when calling them through a central function.
Comment 1 Toni Röhmeyer univentionstaff 2020-09-04 15:06:57 CEST
Fixed Issue with commit

commit dd7f2767520da24d86895a4560344a92cebed599
Bug #51500: fixed Result/PreRead-Hook execution with supports_dry_run

on branch troehmey/bug51500_resulthook_supports_dry_run.


Solution was tested for user massimport with PreReadPyHook and ResultPyHook.
Comment 2 Toni Röhmeyer univentionstaff 2020-09-04 15:24:05 CEST
Added commit

commit af63d994f88a5270389361ff9fdd24a8f05907b1
Bug #51500: changed name of used kwarg: filter_func -> supports_dry_run


In order to avoid that a kwarg "filter_func" is already used somewhere else, I changed it to "supports_dry_run".
Comment 3 Tobias Wenzel univentionstaff 2020-09-17 13:26:38 CEST
QA

ResultPyHook -> OK
before modifications -> postresulthook is called if supports_dry_run is set to False
after modifications -> hook is skipped if supports_dry_run is set to False

Did you also check other ImportPyHook classes like PreReadPyHook & PostReadPyHook?

I got the following:

PreReadPyHook is not called if called if supports_dry_run is set to false, but self.dry_run is False with -n

PostReadPyHook is still called if supports_dry_run is set to False
Comment 4 Daniel Tröder univentionstaff 2020-11-20 11:04:15 CET
* In commit f853f7d3b9d the called hook in UserImport.read_input() was changed from "all_entries_read" to "post_read". Those are two different hook functions.

* Please revert af63d994f. The (kw)argument was a general filter function on purpose. A function that checks if a hook should be executed could check different scenarios, not just the dry-run setting.

* There is generally no need to modify the hook loading mechanism. It must just be invoked correctly.

* Please write two unittests: they should each install a PostReadPyHook and load it. One PostReadPyHook should support dry-run the other not. When executed, they should do something (or not) so it can be verified that they ran (or not).