Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Plagiarism
    • Labels:
    • Testing Instructions:
      Hide

      No Plagiarism plugins exist in core so it is sufficient to test a user submission to ensure that no errors occur.

      • Create a new assignment using online text type. - make sure no errors occur on editing page.
      • login as a student and submit some content - make sure no errors occur.
      • Login as teacher and view submissions page - make sure no errors occur.
      Show
      No Plagiarism plugins exist in core so it is sufficient to test a user submission to ensure that no errors occur. Create a new assignment using online text type. - make sure no errors occur on editing page. login as a student and submit some content - make sure no errors occur. Login as teacher and view submissions page - make sure no errors occur.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      master_MDL-34131

      Description

      Add the plagiarism api hooks to the new assignment with online text.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            kanikagoyal Kanika Goyal added a comment -

            Hi Dan,

            Here is the possible solution - https://github.com/kanikagoyal/moodle/compare/master_MDL-34131_add_plagiarism_api_support_for_new_online_assignment

            Please have a look and let me know what all changes are required.

            Thanks,
            Kanika

            Show
            kanikagoyal Kanika Goyal added a comment - Hi Dan, Here is the possible solution - https://github.com/kanikagoyal/moodle/compare/master_MDL-34131_add_plagiarism_api_support_for_new_online_assignment Please have a look and let me know what all changes are required. Thanks, Kanika
            Hide
            danmarsden Dan Marsden added a comment -

            passing this up for peer review - would be good to get someone elses eyes on this too.

            Show
            danmarsden Dan Marsden added a comment - passing this up for peer review - would be good to get someone elses eyes on this too.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Dan and Kanika,
            A few questions:-

            1. plagiarism_print_disclosure is not needed?
            2. Shouldn't we process the file attachments as well?

            The patch looks great.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Dan and Kanika, A few questions:- plagiarism_print_disclosure is not needed? Shouldn't we process the file attachments as well? The patch looks great. Thanks
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Ankit - print_disclosure should be there already as part of the normal assignment types.

            Yes - attachements should be there, I think there's been a miscommunication between me and Kanika on the requirements here - Kanika I'll send you an e-mail about it.

            Show
            danmarsden Dan Marsden added a comment - Thanks Ankit - print_disclosure should be there already as part of the normal assignment types. Yes - attachements should be there, I think there's been a miscommunication between me and Kanika on the requirements here - Kanika I'll send you an e-mail about it.
            Hide
            kanikagoyal Kanika Goyal added a comment -

            Hi Dan,

            Updated the link. Sorry for the delay.
            Please review it again.

            Thanks,
            Kanika

            Show
            kanikagoyal Kanika Goyal added a comment - Hi Dan, Updated the link. Sorry for the delay. Please review it again. Thanks, Kanika
            Hide
            danmarsden Dan Marsden added a comment -

            great - looks good to me, bouncing up for integration - thanks.

            Show
            danmarsden Dan Marsden added a comment - great - looks good to me, bouncing up for integration - thanks.
            Hide
            nebgor Aparup Banerjee added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            nebgor Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Hi,

            Damyon is the component maintainer of mod_assign and hasn't been involved in this issue, so pinging him for a +1.

            thanks

            Show
            poltawski Dan Poltawski added a comment - Hi, Damyon is the component maintainer of mod_assign and hasn't been involved in this issue, so pinging him for a +1. thanks
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Dan, I had missed this because of the status.

            Comments below:

               $files = $fs->get_area_files($this->assignment->get_context()->id, 'assignsubmission_file', ASSIGNSUBMISSION_FILE_FILEAREA, $submission->id, "id", false);

            This is wrong - the online text plugin should only be dealing with it's own files (linked images etc).

            Should be:

               $files = $fs->get_area_files($this->assignment->get_context()->id, 'assignsubmission_onlinetext', ASSIGNSUBMISSION_ONLINETEXT_FILEAREA, $submission->id, "id", false);

            Also - the 'content' parameter passed to get_links should be trimmed and formatted (at least this is what the old onlinetext assignment does).

            From the old assignment:

            'content' => trim(format_text($submission->data1, $submission->data2, array('context' => $context))), 

            Show
            damyon Damyon Wiese added a comment - Thanks Dan, I had missed this because of the status. Comments below: $files = $fs->get_area_files($this->assignment->get_context()->id, 'assignsubmission_file', ASSIGNSUBMISSION_FILE_FILEAREA, $submission->id, "id", false); This is wrong - the online text plugin should only be dealing with it's own files (linked images etc). Should be: $files = $fs->get_area_files($this->assignment->get_context()->id, 'assignsubmission_onlinetext', ASSIGNSUBMISSION_ONLINETEXT_FILEAREA, $submission->id, "id", false); Also - the 'content' parameter passed to get_links should be trimmed and formatted (at least this is what the old onlinetext assignment does). From the old assignment: 'content' => trim(format_text($submission->data1, $submission->data2, array('context' => $context))),
            Hide
            damyon Damyon Wiese added a comment -

            Other than those issues I am happy with this patch.

            Regards, Damyon

            Show
            damyon Damyon Wiese added a comment - Other than those issues I am happy with this patch. Regards, Damyon
            Hide
            damyon Damyon Wiese added a comment -

            Actually one more thing (possibly should be a different issue). Can the list of valid parameters for get_links be documented somewhere? The array is passed directly to the plagiarism plugin(s) and as there are no plugins in core - and the parameters are not listed on the dev page, it is only possible to guess the parameters by looking at the code in the old assignment module.

            Show
            damyon Damyon Wiese added a comment - Actually one more thing (possibly should be a different issue). Can the list of valid parameters for get_links be documented somewhere? The array is passed directly to the plagiarism plugin(s) and as there are no plugins in core - and the parameters are not listed on the dev page, it is only possible to guess the parameters by looking at the code in the old assignment module.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Damyon, I'm reopening this based on those comments which need to be addressed.

            Show
            poltawski Dan Poltawski added a comment - Thanks Damyon, I'm reopening this based on those comments which need to be addressed.
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Dan/Damyon - whoops, will try to add Damyon to any future mod/assign stuff - Kanika can you please make the changes Damyon suggests? - thanks!

            Show
            danmarsden Dan Marsden added a comment - Thanks Dan/Damyon - whoops, will try to add Damyon to any future mod/assign stuff - Kanika can you please make the changes Damyon suggests? - thanks!
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            kanikagoyal Kanika Goyal added a comment -

            Thanks Damyon for pointing that out. I have updated the link.

            Thanks,
            Kanika

            Show
            kanikagoyal Kanika Goyal added a comment - Thanks Damyon for pointing that out. I have updated the link. Thanks, Kanika
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Rosie - Damyon can you please take a look at the updated patch and confirm you're ok for this to go up for integration?

            thanks!

            Show
            danmarsden Dan Marsden added a comment - Thanks Rosie - Damyon can you please take a look at the updated patch and confirm you're ok for this to go up for integration? thanks!
            Hide
            damyon Damyon Wiese added a comment -

            Those changes look good. +1 from me - thanks!

            Show
            damyon Damyon Wiese added a comment - Those changes look good. +1 from me - thanks!
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Damyon

            Show
            danmarsden Dan Marsden added a comment - thanks Damyon
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks guys, i've integrated this now.

            Show
            poltawski Dan Poltawski added a comment - Thanks guys, i've integrated this now.
            Hide
            fred Frédéric Massart added a comment -

            Hi guys, while testing I had this notice on the page following the student submission.

            Notice: Trying to get property of non-object in /home/fred/www/repositories/testing_master/moodle/mod/assign/submission/onlinetext/locallib.php on line 140
             
            Call Stack:
                0.0003     661944   1. {main}() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:0
                0.1833   54305056   2. assign->view() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:53
                0.1833   54305416   3. assign->process_save_submission() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:303
                0.2467   56076288   4. assign_submission_onlinetext->save() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:2827
             
             
            Notice: Trying to get property of non-object in /home/fred/www/repositories/testing_master/moodle/mod/assign/submission/onlinetext/locallib.php on line 140
             
            Call Stack:
                0.0003     661944   1. {main}() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:0
                0.1833   54305056   2. assign->view() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:53
                0.1833   54305416   3. assign->process_save_submission() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:303
                0.2467   56076288   4. assign_submission_onlinetext->save() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:2827

            Apart from that, the test is all good. Should I pass, fail or wait for a quick fix?

            Show
            fred Frédéric Massart added a comment - Hi guys, while testing I had this notice on the page following the student submission. Notice: Trying to get property of non-object in /home/fred/www/repositories/testing_master/moodle/mod/assign/submission/onlinetext/locallib.php on line 140   Call Stack: 0.0003 661944 1. {main}() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:0 0.1833 54305056 2. assign->view() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:53 0.1833 54305416 3. assign->process_save_submission() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:303 0.2467 56076288 4. assign_submission_onlinetext->save() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:2827     Notice: Trying to get property of non-object in /home/fred/www/repositories/testing_master/moodle/mod/assign/submission/onlinetext/locallib.php on line 140   Call Stack: 0.0003 661944 1. {main}() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:0 0.1833 54305056 2. assign->view() /home/fred/www/repositories/testing_master/moodle/mod/assign/view.php:53 0.1833 54305416 3. assign->process_save_submission() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:303 0.2467 56076288 4. assign_submission_onlinetext->save() /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php:2827 Apart from that, the test is all good. Should I pass, fail or wait for a quick fix?
            Hide
            danmarsden Dan Marsden added a comment -

            I'd consider that a fail but I should be able to push a quick fix in about 3 hours when I get back to my desktop machine unless someone else beats me to it

            Show
            danmarsden Dan Marsden added a comment - I'd consider that a fail but I should be able to push a quick fix in about 3 hours when I get back to my desktop machine unless someone else beats me to it
            Hide
            danmarsden Dan Marsden added a comment -

            just pushed a fix - can an integrator please pull in the updated code? - it was using the wrong vars when triggering the event. thanks!

            Show
            danmarsden Dan Marsden added a comment - just pushed a fix - can an integrator please pull in the updated code? - it was using the wrong vars when triggering the event. thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            done

            Show
            poltawski Dan Poltawski added a comment - done
            Hide
            fred Frédéric Massart added a comment -

            Test successful on master. Cheers!

            Show
            fred Frédéric Massart added a comment - Test successful on master. Cheers!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Fixed STOP Closed STOP Thanks STOP

            Yay, imagination! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12