User Feature: Session Reminder and Configuration as a User Default Settings

[fhelfer]

This PR adds a new User Default Setting to enable/disable the option to configure a Session Reminder time in User Settings.

Feature Wiki: https://docu.ilias.de/goto_docu_wiki_wpage_8270_1357.html

[mjansenDatabay]

Thx @fhelfer for your contribution.

I added some minor remarks for the changes in the the Authentication component.

We should have a chat about the naming of the *LeadTime-methods, which I am not so happy with.

1. getGlobalSessionReminderLeadTime
2. getAcceptableLeadTime
3. getLocalSessionLeadTime
4. getMaxLeadTime

The first and last names are more or less self-explanatory, but: What is the concept of "acceptable"? "acceptable" for whom? Do you mean something like "effective", and what do you mean by "local"? Do you mean something like individual?

Best regards,
Michael

[kergomard]

Hi @fhelfer

Thank you very much for this PR! I will wait with merging until this feature pr is merged #8116, this will also mean, that you will need to rebase to consolidate the user update steps. So you might want to wait until the merge, before working on this again. When you consolidate this, please make sure to not add use statements for classes in the global namespace.

I would like the following changes, but let me know, if you see something wrong with them:

• I don't like this getter here. "getSelect" has an abysmal semantic value already, but "getInput" is even worse. I would kindly ask you to change this to something more specific and to only retrieve the 'default_session_reminder', this way at least I get, what we are doing here.
• On the other hand this here seems oddly over specific: We have "selection", and then "session_reminder_lead_time". Shouldn't this rather be "numeric"?

One very small additional change:

• Could you please remove the camelCase here as you are touching this variable anyway.

Other then this I think things are fine. I will make a little smoke test after the changes and the rebase.

Thanks again and best,
@kergomard

[kergomard]

Thank you very much for the fixes @fhelfer !

I now did some smoke tests and sadly there is still something wrong (sorry for missing it, when reading through the code):
-Please fix the update step, as you are inserting 'session_reminder_lead_time' twice, if 'session_reminder_enabled' has a value, this throws a duplicate primary key error. Please also wrap the whole step into an if-statement to only be executed if there is not already a 'session_reminder_lead_time' value, this way it is made easier to rerun the step on failure.

I assume the changes in Auth to be covered by the thumbs-up from @mjansenDatabay and will thus merge as soon as you have fixed this,
@kergomard

[mjansenDatabay]

I assume the changes in Auth to be covered by the thumbs-up from @mjansenDatabay

This is right

[fhelfer]

Hey @kergomard,

would you have another look at it. I also added dbupdatestep to remove session_reminder_enabled from usr_pref.
If the changes are fine please go ahead and merge

Thx and Best @fhelfer

[kergomard]

Thank you very much @fhelfer !
This looks fine now.
For the record: Failing tests are not because of this PR.

[oliversamoila]

Thank you very much, @kergomard, @fhelfer and @mjansenDatabay 😊