Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            gerry Gerard Caulfield added a comment -

            Discovered by Eloy

            Show
            gerry Gerard Caulfield added a comment - Discovered by Eloy
            Hide
            gerry 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
            gerry 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
            nebgor 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
            nebgor 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
            gerry Gerard Caulfield added a comment -

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

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

            Changes made. You were 100% correct Sam, thanks

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

            Apu, are you able to check over this again?

            Show
            gerry Gerard Caulfield added a comment - Apu, are you able to check over this again?
            Hide
            nebgor 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
            nebgor 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
            gerry 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
            gerry 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
            nebgor 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
            nebgor 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
            gerry Gerard Caulfield added a comment -

            Updating branches for Apu's first and second suggested changes

            Show
            gerry Gerard Caulfield added a comment - Updating branches for Apu's first and second suggested changes
            Hide
            samhemelryk 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
            samhemelryk 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
            gerry Gerard Caulfield added a comment -

            Thanks Sam, looking into this now.

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

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

            Show
            gerry Gerard Caulfield added a comment - Updated patches, thanks again for pointing out the doc problems.
            Hide
            samhemelryk 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
            samhemelryk 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
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  28/Nov/11