Wunderbyte-GmbH / moodle-mod_booking

Moodle Booking Module
https://www.wunderbyte.at
21 stars 38 forks source link

Create behat tests for booking rules #445

Open semteacher opened 3 months ago

semteacher commented 3 months ago

Create behat tests for booking rules

======================================== phpunit tests

semteacher commented 3 months ago

An issue on rule's creation via test:

Exception - Warning: Undefined property: stdClass::$id in [dirroot]/mod/booking/classes/booking_rules/rules/rule_react_on_event.php on line 146
code: generalexceptionmessage
* line 157 of /lib/behat/lib.php: Exception thrown
* line 146 of /mod/booking/classes/booking_rules/rules/rule_react_on_event.php: call to behat_error_handler()
* line 202 of /mod/booking/classes/booking_rules/rules_info.php: call to mod_booking\booking_rules\rules\rule_react_on_event->save_rule()
* line 74 of /mod/booking/classes/form/rulesform.php: call to mod_booking\booking_rules\rules_info::save_booking_rule()
* line 75 of /lib/form/classes/external/dynamic_form.php: call to mod_booking\form\rulesform->process_dynamic_submission()
* line ? of unknownfile: call to core_form\external\dynamic_form::execute()
* line 253 of /lib/external/classes/external_api.php: call to call_user_func_array()
* line 83 of /lib/ajax/service.php: call to core_external\external_api::call_external_function()

Manually passed (beacuse it is only warning)

bernhard-wunderbyte commented 3 months ago

I added a check for !empty here, which should fix this.

semteacher commented 3 months ago

Does the booking rule with the following settings

Image

for the following booking option

Image

expected to be working? No events in the logs...

semteacher commented 3 months ago

UPD: test interupted because of error in musi during cron execution:

Scheduled task failed: Create the daily SAP files (local_musi\task\create_sap_files),Warning: Undefined variable $gatewayspart in /var/www/html/local/musi/classes/sap_daily_sums.php on line 107
Backtrace:
* line 107 of /local/musi/classes/sap_daily_sums.php: call to behat_error_handler()
* line 599 of /local/musi/classes/sap_daily_sums.php: call to local_musi\sap_daily_sums::generate_sap_text_file_for_date()
* line 42 of /local/musi/classes/task/create_sap_files.php: call to local_musi\sap_daily_sums::create_sap_files_from_date()
* line 405 of /lib/classes/cron.php: call to local_musi\task\create_sap_files->execute()
* line 208 of /lib/classes/cron.php: call to core\cron::run_inner_scheduled_task()
* line 125 of /lib/classes/cron.php: call to core\cron::run_scheduled_tasks()
* line 80 of /admin/cron.php: call to core\cron::run_main_process()

However, cron itself being completed with no record in adhock task's table remaining

semteacher commented 3 months ago

UPD: firther debugging reveals issue in email sending query

Adhoc task complete: core\task\send_login_notifications
Execute adhoc task: mod_booking\task\send_mail_by_rule_adhoc
Adhoc task id: 368001
Adhoc task custom data: {"rulename":"rule_daysbefore","ruleid":"497000","rulejson":"{\"conditionname\":\"select_users\",\"conditiondata\":{\"userids\":[\"2\"]},\"name\":\"2daybefore\",\"actionname\":\"send_mail\",\"actiondata\":{\"subject\":\"1daybefore\",\"template\":\"will start tomorrow\",\"templateformat\":\"1\"},\"rulename\":\"rule_daysbefore\",\"ruledata\":{\"days\":\"1\",\"datefield\":\"coursestarttime\"}}","userid":"2","optionid":"478002","cmid":"238000","customsubject":"1daybefore","custommessage":"will start tomorrow"}
... started 09:42:35. Current memory use 7.0 MB.
send_mail_by_rule_adhoc task: sending mail for option 478002 to user 2
... used 3 dbqueries
... used 0.0095210075378418 seconds
Adhoc task failed: mod_booking\task\send_mail_by_rule_adhoc,Error reading from database (ERROR:  missing FROM-clause entry for table "ud"
LINE 5: ...coursestarttime IS NOT NULL   AND bo.id = $1  AND ud.userid ...
                                                             ^
SELECT  CONCAT(bo.id, '-', u.id) uniqueid, bo.id optionid, cm.id cmid, bo.coursestarttime datefield, u.id userid FROM b_booking_options bo
                    JOIN b_course_modules cm
                    ON cm.instance = bo.bookingid
                    JOIN b_modules m
                    ON m.name = 'booking' AND m.id = cm.module JOIN b_user u ON 1 = 1  WHERE  bo.coursestarttime IS NOT NULL   AND bo.id = $1  AND ud.userid = $2  AND u.id = $3
[array (
  0 => 478002,
  1 => 2,
  2 => '2',
)])
Debug info:
ERROR:  missing FROM-clause entry for table "ud"
LINE 5: ...coursestarttime IS NOT NULL   AND bo.id = $1  AND ud.userid ...
                                                             ^
SELECT  CONCAT(bo.id, '-', u.id) uniqueid, bo.id optionid, cm.id cmid, bo.coursestarttime datefield, u.id userid FROM b_booking_options bo
                    JOIN b_course_modules cm
                    ON cm.instance = bo.bookingid
                    JOIN b_modules m
                    ON m.name = 'booking' AND m.id = cm.module JOIN b_user u ON 1 = 1  WHERE  bo.coursestarttime IS NOT NULL   AND bo.id = $1  AND ud.userid = $2  AND u.id = $3
[array (
  0 => 478002,
  1 => 2,
  2 => '2',
)]
Backtrace:
* line 293 of /lib/dml/moodle_read_slave_trait.php: call to moodle_database->query_end()
* line 358 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->read_slave_query_end()
* line 1044 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
* line 317 of /mod/booking/classes/booking_rules/rules/rule_daysbefore.php: call to pgsql_native_moodle_database->get_records_sql()
* line 239 of /mod/booking/classes/booking_rules/rules/rule_daysbefore.php: call to mod_booking\booking_rules\rules\rule_daysbefore->get_records_for_execution()
* line 85 of /mod/booking/classes/task/send_mail_by_rule_adhoc.php: call to mod_booking\booking_rules\rules\rule_daysbefore->check_if_rule_still_applies()
* line 508 of /lib/classes/cron.php: call to mod_booking\task\send_mail_by_rule_adhoc->execute()
* line 302 of /lib/classes/cron.php: call to core\cron::run_inner_adhoc_task()
* line 128 of /lib/classes/cron.php: call to core\cron::run_adhoc_tasks()
* line 80 of /admin/cron.php: call to core\cron::run_main_process()
semteacher commented 3 months ago

UPD: an additional issue - "invalidtaskid" appears via manual test... Cannot even explain

Image

bernhard-wunderbyte commented 3 months ago

@georgmaisser Can you help Andrii with this?

bernhard-wunderbyte commented 3 months ago

HI @semteacher - I just added a fix for this in local_musi:

Scheduled task failed: Create the daily SAP files (local_musi\task\create_sap_files),Warning: Undefined variable $gatewayspart in /var/www/html/local/musi/classes/sap_daily_sums.php on line 107

bernhard-wunderbyte commented 3 months ago

ERROR: missing FROM-clause entry for table "ud" @georgmaisser The reason for this is that the part

if (!empty($userid)) {
$anduserid = "AND ud.userid = :userid";
$params['userid'] = $userid;
}

is defined in rule_daysbefore.php but it should be defined in the conditions match_userprofilefield.php and enter_userprofilefield.php.

So this needs to be migrated from the rule into the conditions. However, we still need to check if the other (non-profile-field) conditions still work if we remove it from the rule.

bernhard-wunderbyte commented 3 months ago

@semteacher => @georgmaisser will fix this soon

georgmaisser commented 3 months ago

@semteacher First, deactivate the tasks for local_musi http://10.111.0.2:8000/admin/tool/task/scheduledtasks.php

I'll look into the rest.

semteacher commented 3 months ago

@georgmaisser Thank you. Latest commit looks like solved musi issue. However, mentioned above SQL error remains evry time I try to run cron wtih that rule's task awaiting of execution

Image

georgmaisser commented 3 months ago

Hi Andrii,

I am working on the rules now and I will add here some notes on what is and what is not working. I will edit this post. checked statements are working, unchecked are not working.

semteacher commented 3 months ago

Another error occurs after installation up to https://github.com/Wunderbyte-GmbH/moodle-mod_booking/commit/ea119cd41692680b8cd596ef5d330196afb6b2fa on save of boking optrion when global rule exists

ERROR:  column u.userid does not exist
LINE 1: SELECT   '' || bo.id || '-' || u.userid  uniqueid, bo.id opt...
                                       ^
SELECT   '' || bo.id || '-' || u.userid  uniqueid, bo.id optionid, cm.id cmid, bo.coursestarttime datefield, u.id userid FROM m_booking_options bo
                    JOIN m_course_modules cm
                    ON cm.instance = bo.bookingid
                    JOIN m_modules m
                    ON m.name = 'booking' AND m.id = cm.module JOIN m_user u ON 1 = 1  WHERE  bo.coursestarttime >= ( $1 + (86400 * $2 ))  AND bo.id = $3    AND u.id = $4
[array (
  0 => 1711731883,
  1 => 1,
  2 => 267,
  3 => '2',
)]
Error code: dmlreadexception
* line 494 of /lib/dml/moodle_database.php: dml_read_exception thrown
* line 293 of /lib/dml/moodle_read_slave_trait.php: call to moodle_database->query_end()
* line 358 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->read_slave_query_end()
* line 1044 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
* line 317 of /mod/booking/classes/booking_rules/rules/rule_daysbefore.php: call to pgsql_native_moodle_database->get_records_sql()
* line 206 of /mod/booking/classes/booking_rules/rules/rule_daysbefore.php: call to mod_booking\booking_rules\rules\rule_daysbefore->get_records_for_execution()
* line 250 of /mod/booking/classes/booking_rules/rules_info.php: call to mod_booking\booking_rules\rules\rule_daysbefore->execute()
* line 3413 of /mod/booking/classes/booking_option.php: call to mod_booking\booking_rules\rules_info::execute_rules_for_option()
* line 214 of /mod/booking/classes/form/option_form.php: call to mod_booking\booking_option::update()
* line 75 of /lib/form/classes/external/dynamic_form.php: call to mod_booking\form\option_form->process_dynamic_submission()
* line ? of unknownfile: call to core_form\external\dynamic_form::execute()
* line 253 of /lib/external/classes/external_api.php: call to call_user_func_array()
* line 83 of /lib/ajax/service.php: call to core_external\external_api::call_external_function()
semteacher commented 3 months ago

in new behat scenario: Booking rules: create booking rule for teacher removal event The following error appears when teacher created alonf with booking option via DB generator method has to be removed

Exception - Warning: Undefined array key "deduction-teacherid-319000" in [dirroot]/mod/booking/classes/form/editteachersforoptiondate_form.php on line 374

* line 157 of /lib/behat/lib.php: Exception thrown
* line 374 of /mod/booking/classes/form/editteachersforoptiondate_form.php: call to behat_error_handler()
* line 674 of /lib/formslib.php: call to mod_booking\form\editteachersforoptiondate_form->validation()
* line 610 of /lib/formslib.php: call to moodleform->validate_defined_fields()
* line 73 of /lib/form/classes/external/dynamic_form.php: call to moodleform->is_validated()
* line ? of unknownfile: call to core_form\external\dynamic_form::execute()
* line 253 of /lib/external/classes/external_api.php: call to call_user_func_array()
* line 83 of /lib/ajax/service.php: call to core_external\external_api::call_external_function()

When teachers being added / removed manually - no error. Dedicated tests related to techer substiting also passed OK... However, during maunal test "deduction" is accessible if teacher assigned on option level

Image

When during automate testing section always emply.

Image

georgmaisser commented 3 months ago

@semteacher This is linked to the dates of the semester.

For past dates, the teachers are not automatically added to the dates, for future dates they are.

If you want to add a Deduction in the second case, where you see the "all teachers were present at this date" message, you first need to remove the current teacher and set another one. than save and open the modal again.

Bildschirmfoto 2024-04-02 um 20 57 06
georgmaisser commented 3 months ago

@semteacher I also fixed a few smaller bugs for the "Booking rules: create booking rule for teacher removal even" behat.

It goes now until the cron is executed. there seems to be, at least for me locally, a problem, but this might be linked to other plugins even.

semteacher commented 3 months ago

@georgmaisser Issue solved, behat test finalized. Thank you! PS: My doubts were exectly if isset() could be used in that case of validation or if dedication fields had to be mandatory...

semteacher commented 3 months ago

Another potential issues:

  1. For the event rule config

Image

Image

bernhard-wunderbyte commented 3 months ago

@semteacher This is, how it looks on my local container - so the fields should be there actually: Bildschirmfoto 2024-04-04 um 11 38 41

semteacher commented 3 months ago

Question. How much test necessary for booking rules? Even for events - it looks like a lot couldb be prepared due to combinations are possible. At the moment 3 test covers the following events

Image

Image

bernhard-wunderbyte commented 3 months ago

I think, we should have a test for every condition but we do not need a separate test for every role. So we only need a case for "Users who booked" currently. The others are currently quite unlikely. So if we really need them, we'll cover them later.

bernhard-wunderbyte commented 3 months ago

However, we should cover more than one condition (not roles).

semteacher commented 3 months ago

@bernhard-wunderbyte Thank you!

  1. On profile fieds - I found that froget to create since local dev was updated. Now looks OK image
  2. Speaking of coverage. OK, will try to add more... But my always' concern is overal time for behat tests....
bernhard-wunderbyte commented 3 months ago

We can still take them out later, once booking rules are stable.

semteacher commented 3 months ago

Have added behat Scenario: Booking rules: create booking rule for option cancellation event and notify user matching profile field value But it seems that events related to user profile fields "conditionname":"enter_userprofilefield" and "conditionname":"match_userprofilefield" not being processed by ovserver - method execute() of rule_react_on_event class not being called. UPD: not an issue

semteacher commented 3 months ago

"react on event" has been done since https://github.com/Wunderbyte-GmbH/moodle-mod_booking/commit/b9e76609a729021c4e7ac8f1c85b0886b9ebb23c

bernhard-wunderbyte commented 3 months ago

There is a new rule action "send copy of mail" and 2 new events for it: "custom_message_sent" and "custom_bulk_message_sent". Can you write tests for these also?

semteacher commented 3 months ago

"react on event" has reactivated because :

there is a new rule action "send copy of mail" and 2 new events for it: "custom_message_sent" and "custom_bulk_message_sent". 
custom_message_sent => Gets triggered when a teacher selects a user and writes a custom message.
custom_bulk_message_sent => Gets triggered when a teacher selects at least 3 users which are at least 75% of all users of a booking option and writes a custom message to all of them.
send_copy_of_mail => new rule action that sends a copy of the original mail to the users selected by the condition (e.g. teachers of the booking option)
with the send_copy_of_mail rule action you can enter additional text to be added to the subject and message of the original e-mail
semteacher commented 3 months ago
  1. "react on event" complete again
  2. Trying back to "n days before". Got error
 Moodle exception: Exception - Warning: Undefined property: stdClass::$datafromevent in [dirroot]/mod/booking/classes/booking_rules/actions/send_mail.php on line 72 
        Stack trace: Error code: generalexceptionmessage 
        * line 157 of /lib/behat/lib.php: Exception thrown 
        * line 72 of /mod/booking/classes/booking_rules/actions/send_mail.php: call to behat_error_handler() 
        * line 210 of /mod/booking/classes/booking_rules/rules/rule_daysbefore.php: call to mod_booking\booking_rules\actions\send_mail->set_actiondata_from_json() 
        * line 255 of /mod/booking/classes/booking_rules/rules_info.php: call to mod_booking\booking_rules\rules\rule_daysbefore->execute() 
        * line 3442 of /mod/booking/classes/booking_option.php: call to mod_booking\booking_rules\rules_info::execute_rules_for_option() 
        * line 214 of /mod/booking/classes/form/option_form.php: call to mod_booking\booking_option::update() 
        * line 75 of /lib/form/classes/external/dynamic_form.php: call to mod_booking\form\option_form->process_dynamic_submission() 

Attempt to debug failed - entry does not intercepted (on observer level - does intercepted but fells into timeout).

bernhard-wunderbyte commented 3 months ago

Thank you for finding this. With "n days before" we have no "datafromevent" because there is no event. I'll fix it.

bernhard-wunderbyte commented 3 months ago

Fixed with commit 64104be08faec21a1ae5ace23a6322cc3acd77b8

semteacher commented 1 month ago

UPD about booking rules for installments

Image

If you add a rule for installments like in the screenshot, the task is scheduled immediately with the nextruntime timestamp according to the setting - in this case 2 days before each installment. when this moment is reached and cron starts to execute the rule, it is first checked if the installment is still open. if for example a user paid the installment 3 days before it was due, he will not recieve the mail. we already implemented this check. if you need data for the phpunit tests, have a look in json of shoppingcarthistory for bookings with installments. here is an example: {"installments":{"originalprice":100,"itemname":"Kurs mit Ratenzahlung","initialpayment":20,"currency":"EUR","payments":[{"id":0,"date":"21 July 2024","price":60,"currency":"EUR","paid":0,"timestamp":1721591480},{"id":1,"date":"5 September 2024","price":60,"currency":"EUR","paid":0,"timestamp":1725522680}],"installments":"2","installmentslinkeditems":[{"itemname":"Kind","originalprice":50,"initialpayment":10,"currency":"EUR"}]}} paid will be set to 1 and price column updated when installment is paid.