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
    • Rank:
      46072

      Description

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

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

        Activity

        Hide
        Kirill Astashov added a comment -

        Attaching patch.

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

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

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

        Thanks Kirill,

        I have marked this for peer review.

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Thanks Kirill, I have marked this for peer review. Regards, Damyon
        Hide
        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 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
        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
        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 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 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 Wiese added a comment -

        Two thumbs up from me!

        Show
        Damyon Wiese added a comment - Two thumbs up from me!
        Hide
        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
        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 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 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 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 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 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 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
        Dan Poltawski added a comment -

        OK, added to integration_held queue.

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

        Integrated to master only, thanks Damyon.

        (Resolving a minor conflict with Damyons help)

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

        Works as described Thanks Damyon

        Show
        Jason Fowler added a comment - Works as described Thanks Damyon
        Hide
        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
        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: