Issue #81 No failure during duplicate and write error into event #90

TomoTsuyuki · 2023-10-13T00:09:19Z

Implemented to avoid error in the middle of duplication. #81
Event "massaction_duplicated" has been introduced for the logging.

dmitriim · 2023-10-13T00:38:52Z

classes/actions.php

+            try {
+                $duplicatedmod = duplicate_module($modinfo->get_course(), $modinfo->get_cm($cmid));
+            } catch (\Exception $e) {
+                $errors[] = 'cmid:' . $cmid . '(' . $e->getMessage() . ')';


I think it better to fire up an error event here instead of collecting errors and dump them in massaction_duplicated. Otherwise massaction_duplicated message could be very gross.

I push the error detail to a new "failed" event, and make simple for a "duplicate" event.

dmitriim · 2023-10-13T00:40:11Z

classes/event/massaction_duplicated.php

+ * @copyright   2023 Catalyst IT
+ * @license     https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class massaction_duplicated extends base {


We already have this event under massaction block. We don't really need to duplicate it in event name. How about renaming to activities_duplicated or course_modules_duplicated?

I renamed it to "course_modules_duplicated".

dmitriim · 2023-10-13T00:43:38Z

classes/event/massaction_duplicated.php

+        foreach ($this->other['cms'] as $srccm => $dstcm) {
+            $cms[] = 'cmid from \'' . $srccm . '\' to \'' . $dstcm . '\'';
+        }
+        $errormsg = !empty($this->other['errors']) ? " with error '" . implode("','", $this->other['errors']) : "'";


As said before I think better just to log a number of errors rather than each individual error. And have a separate event (e.g. activity_duplication_failed ) for any error message. But it's up to a maintainer to decide.

The error details go to a new "failed" event and the "duplicated" event has just number of failed and cmid.

dmitriim · 2023-10-13T00:44:23Z

classes/event/massaction_duplicated.php

+    public function get_description(): string {
+        $cms = [];
+        foreach ($this->other['cms'] as $srccm => $dstcm) {
+            $cms[] = 'cmid from \'' . $srccm . '\' to \'' . $dstcm . '\'';


would be also useful to see which failed and which succeeded . A stats like 5 completed, 3 failed.

I added total of complete/fail to the description.

dmitriim · 2023-10-13T00:46:51Z

classes/event/massaction_duplicated.php

+    protected function validate_data() {
+        parent::validate_data();
+
+        if (!isset($this->other['cms']) || !is_array($this->other['cms'])) {


should we validate $this->other['errors'] also?

I added the new validation.

dmitriim · 2023-10-13T00:50:32Z

Thanks @TomoTsuyuki for working on it. The code looks good in general. I have seen people copping hundreds of activities. I'm a bit worried how that all will look at the logs screen. I have left my comments.

Also the patch covers just duplicate, what about duplicate_to_course?

TomoTsuyuki · 2023-10-13T05:58:27Z

Thanks @TomoTsuyuki for working on it. The code looks good in general. I have seen people copping hundreds of activities. I'm a bit worried how that all will look at the logs screen. I have left my comments.

Also the patch covers just duplicate, what about duplicate_to_course?

Thanks for your review @dmitriim
I updated the events and support duplicate_to_course.

Please let me know if they are ok.

dmitriim · 2023-10-13T06:09:10Z

classes/actions.php

+            try {
+                $duplicatedmod = duplicate_module($modinfo->get_course(), $modinfo->get_cm($cmid));
+            } catch (\Exception $e) {
+                $errors[$cmid] = 'cmid:' . $cmid . '(' . $e->getMessage() . ')';


any particular reason why you don't fire an event here, but instead later looping through the errors?

I moved the event to catch block.

dmitriim · 2023-10-13T06:24:02Z

classes/actions.php

@@ -276,6 +312,26 @@ public static function duplicate_to_course(array $modules, int $targetcourseid,
                moveto_module($targetmodinfo->get_cm($modid), $targetsection);
            }
        }
+        $event = \block_massaction\event\course_modules_duplicated::create([
+            'context' => \context_course::instance($targetcourseid),


Why do you think we should trigger all those events in a context of a target course and not a source course?

I changed to $sourcecourseid instead of $targecourseid

classes/actions.php

Syxton · 2023-10-24T15:48:59Z

@TomoTsuyuki I'm sorry for all the delays on these proposed fixes. I had to make some decisions on the future direction of the plugin. We have made a fairly major change moving forward in 4.2+ so these changes and the ones in the other PR's will need rebased to the latest master commits. Also, when making these requests, please do not make changes to the version.php and I will alter the version number once we have integrated changes. That way we don't have constant conflicts on version numbers. I understand that for testing the version number might need tweaked to add new settings, lang strings, etc. Again sorry to send you back to development on all this work. Your time invested is really valued. Thank you.

TomoTsuyuki · 2023-10-24T23:08:37Z

Hi @Syxton ,

Thanks for your comment.
I rebased my branch with master, and removed the version.php from the commit.
Are you accepting PRs for the new MOODLE_401_STABLE branch?
If you will maintain only master branch, we need to make our fork to update the MOODLE_401_STABLE branch as our clients are using 4.1 LTS.

Syxton · 2023-10-25T12:28:19Z

@TomoTsuyuki The 4.1 branch still works on 4.2 and 4.3, so I will of course still accept PR's that improve and extend the lifespan of that version of the plugin. Just create another PR for the sub 4.2 changes and submit against MOODLE_401_STABLE

TomoTsuyuki · 2023-10-25T23:48:38Z

Thanks @Syxton , that's good news.
I made PRs for 401 branches.
Please review and let me know if I need to do extra work.

Syxton · 2024-01-08T14:48:15Z

lang/en/block_massaction.php

A few minor things:
When is the event:massaction_duplicated string used?
Can we change the failed text from "Course modules duplicated failed" to "Course modules failed to duplicate"

@TomoTsuyuki just wanted to make sure you got notified on this.

Thanks for your review, @Syxton !

I deleted event:massaction_duplicated and update event:course_modules_duplicated_failed string.

I also rebased by the latest master/MOODLE_401_STABLE branch.

TomoTsuyuki force-pushed the issue81-update-mass-duplicate branch from 71e967f to af294bc Compare October 13, 2023 00:20

dmitriim reviewed Oct 13, 2023

View reviewed changes

TomoTsuyuki force-pushed the issue81-update-mass-duplicate branch from af294bc to 7b080e0 Compare October 13, 2023 05:53

dmitriim reviewed Oct 13, 2023

View reviewed changes

This was referenced Oct 16, 2023

Issue #84 Throw exception for the empty new cmid #91

Merged

Rerunning duplicate_task can cause duplicates #81

Closed

Syxton reviewed Oct 20, 2023

View reviewed changes

classes/actions.php Outdated Show resolved Hide resolved

Syxton linked an issue Oct 24, 2023 that may be closed by this pull request

Rerunning duplicate_task can cause duplicates #81

Closed

TomoTsuyuki force-pushed the issue81-update-mass-duplicate branch from 29ef3dc to 0cd88f4 Compare October 24, 2023 22:49

TomoTsuyuki mentioned this pull request Oct 25, 2023

Issue #81 No failure during duplicate and write error into event #97

Merged

TomoTsuyuki force-pushed the issue81-update-mass-duplicate branch from 0cd88f4 to 39993db Compare November 21, 2023 00:53

Issue Syxton#103 Fix escaped text in dropdown

a34c541

Syxton reviewed Jan 8, 2024

View reviewed changes

Syxton and others added 6 commits January 11, 2024 15:37

Update ci.yml

db149b8

Issue Syxton#81 No failure during duplicate and write error into event

3e889e9

Issue Syxton#81 Add fail event and support duplicate to another course

aabedd6

Issue Syxton#81 Refactor error event trigger

7498ea3

Issue Syxton#81 Fix coding standard issue

d9cd7a1

Issue Syxton#81 Update lang strings

dd9c6f8

TomoTsuyuki force-pushed the issue81-update-mass-duplicate branch from 39993db to dd9c6f8 Compare January 11, 2024 04:39

Merge branch 'master' into issue81-update-mass-duplicate

3003abd

Syxton merged commit 692919f into Syxton:master Jan 11, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #81 No failure during duplicate and write error into event #90

Issue #81 No failure during duplicate and write error into event #90

TomoTsuyuki commented Oct 13, 2023 •

edited

Loading

dmitriim Oct 13, 2023

TomoTsuyuki Oct 13, 2023

dmitriim Oct 13, 2023

TomoTsuyuki Oct 13, 2023

dmitriim Oct 13, 2023

TomoTsuyuki Oct 13, 2023

dmitriim Oct 13, 2023 •

edited

Loading

TomoTsuyuki Oct 13, 2023

dmitriim Oct 13, 2023

TomoTsuyuki Oct 13, 2023

dmitriim commented Oct 13, 2023

TomoTsuyuki commented Oct 13, 2023

dmitriim Oct 13, 2023 •

edited

Loading

TomoTsuyuki Oct 16, 2023

dmitriim Oct 13, 2023

TomoTsuyuki Oct 16, 2023

Syxton commented Oct 24, 2023

TomoTsuyuki commented Oct 24, 2023

Syxton commented Oct 25, 2023

TomoTsuyuki commented Oct 25, 2023

Syxton Jan 8, 2024

Syxton Jan 10, 2024

TomoTsuyuki Jan 11, 2024

TomoTsuyuki Jan 11, 2024

Issue #81 No failure during duplicate and write error into event #90

Issue #81 No failure during duplicate and write error into event #90

Conversation

TomoTsuyuki commented Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitriim Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitriim commented Oct 13, 2023

TomoTsuyuki commented Oct 13, 2023

dmitriim Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Syxton commented Oct 24, 2023

TomoTsuyuki commented Oct 24, 2023

Syxton commented Oct 25, 2023

TomoTsuyuki commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomoTsuyuki commented Oct 13, 2023 •

edited

Loading

dmitriim Oct 13, 2023 •

edited

Loading

dmitriim Oct 13, 2023 •

edited

Loading