Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36587

Do not show 'Download all' link when no submissions have been made (Assignment 2.3)

    Details

    • Testing Instructions:
      Hide

      Test instructions:

      1. In a course with at least one student and one teacher, create an assignment with "File submissions" set to "Yes".
      2. Logged in as a teacher, view the assignment, click on the Grade/view All Submissions link.
      3. Ensure that Download all submissions link is not showing in the settings block.
      4. Ensure that Download all submissions item is not present in the dropdown menu.
      5. Log in as a student, upload a file submission.
      6. Logged in as a teacher, view the assignment, click on the Grade/view All Submissions link.
      7. Ensure that Download all submissions link is showing in the settings block.
      8. Ensure that Download all submissions item is present in the dropdown menu.

      Show
      Test instructions: 1. In a course with at least one student and one teacher, create an assignment with "File submissions" set to "Yes". 2. Logged in as a teacher, view the assignment, click on the Grade/view All Submissions link. 3. Ensure that Download all submissions link is not showing in the settings block. 4. Ensure that Download all submissions item is not present in the dropdown menu. 5. Log in as a student, upload a file submission. 6. Logged in as a teacher, view the assignment, click on the Grade/view All Submissions link. 7. Ensure that Download all submissions link is showing in the settings block. 8. Ensure that Download all submissions item is present in the dropdown menu.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36587-master

      Description

      This is to hide 'Download all' link when no submissions have been made since it produces an empty .zip file in this case.

        Gliffy Diagrams

        1. MDL-36587.patch
          2 kB
          Kirill Astashov
        2. MDL-36587-2.patch
          0.9 kB
          Kirill Astashov

          Activity

          Hide
          badblock Kirill Astashov added a comment -

          Attaching patch.

          Show
          badblock Kirill Astashov added a comment - Attaching patch.
          Hide
          badblock Kirill Astashov added a comment -

          Just noticed there's also a dropdown menu item. Attaching additional patch...

          Show
          badblock Kirill Astashov added a comment - Just noticed there's also a dropdown menu item. Attaching additional patch...
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Kirill,

          I have marked this for peer review.

          Regards, Damyon

          Show
          damyon Damyon Wiese added a comment - Thanks Kirill, I have marked this for peer review. Regards, Damyon
          Hide
          damyon Damyon Wiese added a comment -

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          Hi Kirill,

          The change itself is fine but there are no testing instructions, you should provide this as a git branch with a single commit and the commit message needs the Moodle tracker number - not the number from your client branch. Can you please address these issues and I'll re-review the patch.

          Regards, Damyon

          Show
          damyon Damyon Wiese added a comment - [Y] Syntax [-] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing [-] Security [-] Documentation [N] Git [Y] Sanity check Hi Kirill, The change itself is fine but there are no testing instructions, you should provide this as a git branch with a single commit and the commit message needs the Moodle tracker number - not the number from your client branch. Can you please address these issues and I'll re-review the patch. Regards, Damyon
          Hide
          badblock Kirill Astashov added a comment -

          Hi Damyon,

          Hope you're doing well.

          I've added links to pull master branch and diff URL.
          Testing instructions have been added as well.

          Show
          badblock Kirill Astashov added a comment - Hi Damyon, Hope you're doing well. I've added links to pull master branch and diff URL. Testing instructions have been added as well.
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Kirill

          The testing instructions are good and the git branches make it much easier to look at - I have backported to 2.3 so I'll update the git links to my repository now.

          I also tweaked the commit message to include the component name so the preferred format is:

          MDL-36587 Assignment: blah

          Show
          damyon Damyon Wiese added a comment - Thanks Kirill The testing instructions are good and the git branches make it much easier to look at - I have backported to 2.3 so I'll update the git links to my repository now. I also tweaked the commit message to include the component name so the preferred format is: MDL-36587 Assignment: blah
          Hide
          damyon Damyon Wiese added a comment -

          Two thumbs up from me!

          Show
          damyon Damyon Wiese added a comment - Two thumbs up from me!
          Hide
          poltawski Dan Poltawski added a comment -

          I'm reopening this because it looks to me like there is no count_submissions() function in 2.3! Has this been tested?

          I'm not so sure about the navigation change as this will be introducing an extra query on each page. Is it worth this cost for the navigation?

          Show
          poltawski Dan Poltawski added a comment - I'm reopening this because it looks to me like there is no count_submissions() function in 2.3! Has this been tested? I'm not so sure about the navigation change as this will be introducing an extra query on each page. Is it worth this cost for the navigation?
          Hide
          damyon Damyon Wiese added a comment -

          Rebased both branches and added a commit to backport the missing function to 2.3. Sorry about that one I should not have sent this without testing the backport properly.

          I backported the function rather than adding the SQL inline because it had to go in 2 places - and the proper implementation is non trivial.

          Also note - this backported function is based on the code in MDL-36263 which is waiting for integration review (it needs to only check for submissions from enrolled users).

          Show
          damyon Damyon Wiese added a comment - Rebased both branches and added a commit to backport the missing function to 2.3. Sorry about that one I should not have sent this without testing the backport properly. I backported the function rather than adding the SQL inline because it had to go in 2 places - and the proper implementation is non trivial. Also note - this backported function is based on the code in MDL-36263 which is waiting for integration review (it needs to only check for submissions from enrolled users).
          Hide
          damyon Damyon Wiese added a comment -

          About the extra DB queries - this patch adds 3 queries when the settings navigation is open for the assignment and the current user has mod:grade capability (It is 3 queries because it needs to filter by enrolled students using the current groupmode).

          Overall - the mod_assign uses 9 queries for extend settings navigation (116 total on my install).

          Some other modules:
          Forum: 1 extra query
          Chat: 2 extra queries
          Glossary: 2 extra queries
          Workshop: 0 extra queries

          So - in comparison - the assignment is looking like it is doing too much with the settings navigation.

          I'll update this patch to only apply to the drop down list.

          Show
          damyon Damyon Wiese added a comment - About the extra DB queries - this patch adds 3 queries when the settings navigation is open for the assignment and the current user has mod:grade capability (It is 3 queries because it needs to filter by enrolled students using the current groupmode). Overall - the mod_assign uses 9 queries for extend settings navigation (116 total on my install). Some other modules: Forum: 1 extra query Chat: 2 extra queries Glossary: 2 extra queries Workshop: 0 extra queries So - in comparison - the assignment is looking like it is doing too much with the settings navigation. I'll update this patch to only apply to the drop down list.
          Hide
          damyon Damyon Wiese added a comment -

          Pushed a new fix for this issue to the master branch (removed the change to extend_settings_navigation). I also removed the 23 branches as I think this is an improvement rather than a bug and is not worth backporting (this also means we don't have to backport the count_submissions function).

          Show
          damyon Damyon Wiese added a comment - Pushed a new fix for this issue to the master branch (removed the change to extend_settings_navigation). I also removed the 23 branches as I think this is an improvement rather than a bug and is not worth backporting (this also means we don't have to backport the count_submissions function).
          Hide
          poltawski Dan Poltawski added a comment -

          OK, added to integration_held queue.

          Show
          poltawski Dan Poltawski added a comment - OK, added to integration_held queue.
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated to master only, thanks Damyon.

          (Resolving a minor conflict with Damyons help)

          Show
          poltawski Dan Poltawski added a comment - Integrated to master only, thanks Damyon. (Resolving a minor conflict with Damyons help)
          Hide
          phalacee Jason Fowler added a comment -

          Works as described Thanks Damyon

          Show
          phalacee Jason Fowler added a comment - Works as described Thanks Damyon
          Hide
          poltawski Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          poltawski Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/May/13