Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor 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

          Activity

          Hide
          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
          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
          Dan Marsden added a comment -

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

          Show
          Dan Marsden added a comment - passing this up for peer review - would be good to get someone elses eyes on this too.
          Hide
          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 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
          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
          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
          Kanika Goyal added a comment -

          Hi Dan,

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

          Thanks,
          Kanika

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

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

          Show
          Dan Marsden added a comment - great - looks good to me, bouncing up for integration - thanks.
          Hide
          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
          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
          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
          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 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 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 Wiese added a comment -

          Other than those issues I am happy with this patch.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Other than those issues I am happy with this patch. Regards, Damyon
          Hide
          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 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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Thanks Damyon, I'm reopening this based on those comments which need to be addressed.
          Hide
          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
          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 added a comment -

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

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

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

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - Thanks Damyon for pointing that out. I have updated the link. Thanks, Kanika
          Hide
          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
          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 Wiese added a comment -

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

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

          thanks Damyon

          Show
          Dan Marsden added a comment - thanks Damyon
          Hide
          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
          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
          Dan Poltawski added a comment -

          Thanks guys, i've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks guys, i've integrated this now.
          Hide
          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
          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
          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
          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
          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
          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
          Dan Poltawski added a comment -

          done

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

          Test successful on master. Cheers!

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

          Fixed STOP Closed STOP Thanks STOP

          Yay, imagination! Ciao

          Show
          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: