Moodle
  1. Moodle
  2. MDL-30957

A misleading submitted date shows up on the student view for assignment before the submission takes place

    Details

    • Testing Instructions:
      Hide
      1. Look at the screenshot that is attached to this bug. While running through the following steps you should make sure that the date only appears after you have actually submitted the assignment.
      2. The following test need to be run on 20, 21 and 22
        create an assignments of type "Upload a single file" and "Advanced uploading of files"
      3. Switch your role to student.
      4. Go to upload a file on both assignments.
      5. "Save changes" without choosing a file
      6. Check to make sure the date shown in the screenshot has not appeared.
      7. On both assignments go to upload a file again and this time actually upload a file.
      8. Now submit both assignments
      9. With the patch applied, the date shown in the screenshot should only have appeared after you submitted the assignment.
      Show
      Look at the screenshot that is attached to this bug. While running through the following steps you should make sure that the date only appears after you have actually submitted the assignment. The following test need to be run on 20, 21 and 22 create an assignments of type "Upload a single file" and "Advanced uploading of files" Switch your role to student. Go to upload a file on both assignments. "Save changes" without choosing a file Check to make sure the date shown in the screenshot has not appeared. On both assignments go to upload a file again and this time actually upload a file. Now submit both assignments With the patch applied, the date shown in the screenshot should only have appeared after you submitted the assignment.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      m_MDL-30957_adding_assignment_completion_status_method_and_call
    • Rank:
      37358

      Description

      MDL-29400 Fixes the displaying of a date and message which shows the file has been submitted. However another submitted date (although not labeled as such) still shows up on the student view in the top right header, as can be seen in the attached screenshot.

      Eloy spotted this on 29400, however after looking into the cause and issue, it is different from that in 29400. This issue is also less severe as the date isn't labeled as a submitted date. I am attaching a fix for this bug, however I am encountering notices under certain conditions and so I need to do some more testing to determine if these are related at all to the fix I'm providing. For these reasons I though it would be better to split this out into a separate issue.

        Issue Links

          Activity

          Hide
          Gerard Caulfield added a comment -

          Discovered by Eloy

          Show
          Gerard Caulfield added a comment - Discovered by Eloy
          Hide
          Gerard Caulfield added a comment -

          The notices I was seeing are not due to my patch as Adrian was also able to reproduce them on his instance of the master branch. I suspect the notices are actually related to a much bigger error handling issue, so I'll look into it when I get a chance and submit an issue.

          Putting this patch up for peer review.

          Show
          Gerard Caulfield added a comment - The notices I was seeing are not due to my patch as Adrian was also able to reproduce them on his instance of the master branch. I suspect the notices are actually related to a much bigger error handling issue, so I'll look into it when I get a chance and submit an issue. Putting this patch up for peer review.
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Gerry,

          the patch looks ok to me.

          (sql_compare_text() is for text comparisons within DB , not supplied strings , Happy New Year! :-p )

          Show
          Aparup Banerjee added a comment - - edited Hi Gerry, the patch looks ok to me. (sql_compare_text() is for text comparisons within DB , not supplied strings , Happy New Year! :-p )
          Hide
          Gerard Caulfield added a comment -

          Thanks for peer reviewing this Apu. The code changes to the other branches (added above) where identical.

          Show
          Gerard Caulfield added a comment - Thanks for peer reviewing this Apu. The code changes to the other branches (added above) where identical.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Sam Hemelryk added a comment -

          Hi Gerry,

          I've just been looking at this now.
          Certainly introducing a method to determine is a submission is complete is a smart idea.
          However I think the solution you have at the moment needs more work.
          Presently assignment_base::is_complete having hardcoded checks for the assignment type is just nasty. Certainly that can be avoided by defining a basic check, and then each individual assignment type overriding the basic check if required.
          In this way I'm pretty sure if you pass the submission, or even submissionid OR submission you could avoid that additional DB query as well (the information is pretty much all there in $this and $submission by the looks).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Gerry, I've just been looking at this now. Certainly introducing a method to determine is a submission is complete is a smart idea. However I think the solution you have at the moment needs more work. Presently assignment_base::is_complete having hardcoded checks for the assignment type is just nasty. Certainly that can be avoided by defining a basic check, and then each individual assignment type overriding the basic check if required. In this way I'm pretty sure if you pass the submission, or even submissionid OR submission you could avoid that additional DB query as well (the information is pretty much all there in $this and $submission by the looks). Cheers Sam
          Hide
          Gerard Caulfield added a comment - - edited

          Hi Sam

          I don't have time to look into it just yet, but method overriding sounds like a much better idea. Thanks for the suggestion, I'll update this when I'm next on fixing.

          Gerry

          Show
          Gerard Caulfield added a comment - - edited Hi Sam I don't have time to look into it just yet, but method overriding sounds like a much better idea. Thanks for the suggestion, I'll update this when I'm next on fixing. Gerry
          Hide
          Gerard Caulfield added a comment -

          Changes made. You were 100% correct Sam, thanks

          Show
          Gerard Caulfield added a comment - Changes made. You were 100% correct Sam, thanks
          Hide
          Gerard Caulfield added a comment -

          Apu, are you able to check over this again?

          Show
          Gerard Caulfield added a comment - Apu, are you able to check over this again?
          Hide
          Aparup Banerjee added a comment -

          Hi Gerry ,
          just had a look at your latest patch and heres some notes:

          • mod/assignment/lib.php line 404-405:
            if ($this->is_complete($submission)) {
            // this next line is redundant when is_complete() isn't overridden
            // and even when it is overriden - i'd say let the assignment type be responsible and check it all. 
               if ($submission->timemodified) { 
            
            public function is_complete($submission) {
                return $submission->timemodified;
            }
            
          • is_complete() is that a reference to 'Activity completion' ? if not , i think better to rename that function, is_submitted_with_all_required_data() maybe? *note: i might have a tendency to try to put essays into function names.
          • display_submission() has a 'FILTER_SUBMITTED' which when true the displays assignments 'submitted' based on 's.timemodified > 0' , this may be worth looking further into. (not sure)

          thats all folks!

          Show
          Aparup Banerjee added a comment - Hi Gerry , just had a look at your latest patch and heres some notes: mod/assignment/lib.php line 404-405: if ($ this ->is_complete($submission)) { // this next line is redundant when is_complete() isn't overridden // and even when it is overriden - i'd say let the assignment type be responsible and check it all. if ($submission->timemodified) { public function is_complete($submission) { return $submission->timemodified; } is_complete() is that a reference to 'Activity completion' ? if not , i think better to rename that function, is_submitted_with_all_required_data() maybe? *note: i might have a tendency to try to put essays into function names. display_submission() has a 'FILTER_SUBMITTED' which when true the displays assignments 'submitted' based on 's.timemodified > 0' , this may be worth looking further into. (not sure) thats all folks!
          Hide
          Gerard Caulfield added a comment -

          Hi Apu

          1. Thanks for spotting that
          2. Good point, the entire assignment is not in a complete state (as in marked and no more left to do) it is just the submission that is complete, so your function name is better although I removed the word all as I didn't feel it was necessary is_submitted_with_required_data()
          3. Yeah that is related to the displaying of submissions on the /mod/assignment/submissions.php. I spoke with Michael about whether I should remove non complete submissions from that page and was told not to especially considering they are displayed as drafts on that page.

          I'll make the first two changes and submit for integration unless I hear differently from you.

          Thanks for your help with this Apu

          Gerry

          Show
          Gerard Caulfield added a comment - Hi Apu Thanks for spotting that Good point, the entire assignment is not in a complete state (as in marked and no more left to do) it is just the submission that is complete, so your function name is better although I removed the word all as I didn't feel it was necessary is_submitted_with_required_data() Yeah that is related to the displaying of submissions on the /mod/assignment/submissions.php . I spoke with Michael about whether I should remove non complete submissions from that page and was told not to especially considering they are displayed as drafts on that page. I'll make the first two changes and submit for integration unless I hear differently from you. Thanks for your help with this Apu Gerry
          Hide
          Aparup Banerjee added a comment - - edited

          cool thanks for mentioning 'was told not to especially considering they are displayed as drafts on that page' - yup makes sense for the teacher to see everything even if half submitted.. perhaps a marker/flag/shining-star might be in order here (in the display of submissions) to signal complete from incomplete submissions? o r perhaps its already quite evident.

          I'll leave that to you Gerry since you're in the area (*if creating any issues note that assignment is due for a re-write sometime, so there's probably a meta MDL out there)

          Show
          Aparup Banerjee added a comment - - edited cool thanks for mentioning 'was told not to especially considering they are displayed as drafts on that page' - yup makes sense for the teacher to see everything even if half submitted.. perhaps a marker/flag/shining-star might be in order here (in the display of submissions) to signal complete from incomplete submissions? o r perhaps its already quite evident. I'll leave that to you Gerry since you're in the area (*if creating any issues note that assignment is due for a re-write sometime, so there's probably a meta MDL out there)
          Hide
          Gerard Caulfield added a comment -

          Updating branches for Apu's first and second suggested changes

          Show
          Gerard Caulfield added a comment - Updating branches for Apu's first and second suggested changes
          Hide
          Sam Hemelryk added a comment -

          Hi Gerry,

          I've had a quick look at this, the new function is a huge improvement and appears to be spot on.
          There are however a couple of doc issues to fix up before this goes in:

          1. @param int $submission... Submission is being used as an object surely int is incorrect (in all functions)
          2. Line 403 of lib.php the new comment, there is an alignment issue and there should be a space "// If the submission has been completed"

          They're both trivial so I've left this in integration review, can you please get onto it asap and ping me when the branch has been updated so I can integrate it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Gerry, I've had a quick look at this, the new function is a huge improvement and appears to be spot on. There are however a couple of doc issues to fix up before this goes in: @param int $submission... Submission is being used as an object surely int is incorrect (in all functions) Line 403 of lib.php the new comment, there is an alignment issue and there should be a space "// If the submission has been completed" They're both trivial so I've left this in integration review, can you please get onto it asap and ping me when the branch has been updated so I can integrate it. Cheers Sam
          Hide
          Gerard Caulfield added a comment -

          Thanks Sam, looking into this now.

          Show
          Gerard Caulfield added a comment - Thanks Sam, looking into this now.
          Hide
          Gerard Caulfield added a comment -

          Updated patches, thanks again for pointing out the doc problems.

          Show
          Gerard Caulfield added a comment - Updated patches, thanks again for pointing out the doc problems.
          Hide
          Sam Hemelryk added a comment -

          Thanks Gerry this has been integrated now

          Please note that 2.0.x is now only receiving security and essential fixes. It's no longer required to produce a patch for 2.0 unless the changes are essential.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Gerry this has been integrated now Please note that 2.0.x is now only receiving security and essential fixes. It's no longer required to produce a patch for 2.0 unless the changes are essential. Cheers Sam
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for fixing the Gerry.
          Works Great for me.

          Although found new problem, for upload a single file with resubmit set to no. Student can delete/update the file. Will open another bug for the same.

          Show
          Rajesh Taneja added a comment - - edited Thanks for fixing the Gerry. Works Great for me. Although found new problem, for upload a single file with resubmit set to no. Student can delete/update the file. Will open another bug for the same.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: