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

New grading interface should hide editpdf if unoconv is not enabled

    Details

    • Testing Instructions:
      Hide

      Please test on multiple browsers.

      Note: This test requires your server to be set up with ghostscript so that the grading interface works. If you haven't set it up yet then do Test 2 before setting it up and then complete Test 1 after it's set up.

      Test 1

      1. Create a course some students and an assignment
      2. Log in as a teacher / admin and grade the assignment
        • go to the course > assignments > click grade button
      3. Notice three buttons down the bottom right of the grading interface
      4. Click each of the buttons and confirm that the interface hides the appropriate panels, as per the icons on the buttons.
        • Pressing enter or space on a button with focus should also activate the button
      5. Confirm that you can't tab into a hidden panel
      6. Confirm that you can't navigate into a hidden panel using a screen reader
      7. Narrow the screen to less than 767px wide
      8. Confirm that the review panel has a collapse/expand button up the top left and clicking it collapses/expands the panel.
      9. Change the language to an RTL language
      10. Confirm that the collapse/expand button on the narrow view changes to the top right hand side of the panel.
      11. Confirm that on a full width page in RTL that the layout buttons remain in the same order as they did for LTR (because the layout of the grading interface doesn't change).

      Test 2

      1. Either run this test on a server that doesn't have ghostscript installed / configured or you can edit mod/assign/classes/output/grading_app.php and set $export->showreview = false; around line 124 (after the plugin loop) to simulate the situation.
      2. Load up the grading interface from Test 1
      3. Confirm that no review panel is shown
      4. Confirm that no layout buttons are shown on the bottom right of the page
      Show
      Please test on multiple browsers. Note: This test requires your server to be set up with ghostscript so that the grading interface works. If you haven't set it up yet then do Test 2 before setting it up and then complete Test 1 after it's set up. Test 1 Create a course some students and an assignment Log in as a teacher / admin and grade the assignment go to the course > assignments > click grade button Notice three buttons down the bottom right of the grading interface Click each of the buttons and confirm that the interface hides the appropriate panels, as per the icons on the buttons. Pressing enter or space on a button with focus should also activate the button Confirm that you can't tab into a hidden panel Confirm that you can't navigate into a hidden panel using a screen reader Narrow the screen to less than 767px wide Confirm that the review panel has a collapse/expand button up the top left and clicking it collapses/expands the panel. Change the language to an RTL language Confirm that the collapse/expand button on the narrow view changes to the top right hand side of the panel. Confirm that on a full width page in RTL that the layout buttons remain in the same order as they did for LTR (because the layout of the grading interface doesn't change). Test 2 Either run this test on a server that doesn't have ghostscript installed / configured or you can edit mod/assign/classes/output/grading_app.php and set $export->showreview = false; around line 124 (after the plugin loop) to simulate the situation. Load up the grading interface from Test 1 Confirm that no review panel is shown Confirm that no layout buttons are shown on the bottom right of the page
    • Affected Branches:
      MOODLE_31_STABLE
    • Fixed Branches:
      MOODLE_31_STABLE
    • Pull 3.1 Branch:
    • Pull Master Branch:
      MDL-54165-master

      Description

      Currently this feature requires unoconv 0.7 to work, however most distros are still pointing to 0.6. So, it is not entirely easy to set this feature up. Even after downloading the unoconv file from https://github.com/dagwieers/unoconv I received the error 'unoconv: Cannot find a suitable office installation on your system. ERROR: Please locate your office installation and send your feedback to: http://github.com/dagwieers/unoconv/issues' when trying to run it - which means it requires even further tweaking. I think it is safe to assume that a large majority of sites will not have this feature enabled (at least for some time). So, when teachers view this area for marking they will see a huge dominant white area that serves no use unless they are marking a PDF and will cause a lot of confusion. We should solve this now. Currently if you do not have GS on your site the editpdf is not shown. We should do the same for unoconv if the files being marked are not PDFs.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              marycooch Mary Cooch added a comment -

              A very big +1

              Show
              marycooch Mary Cooch added a comment - A very big +1
              Hide
              damyon Damyon Wiese added a comment -

              After considering it I am OK with this change. +1 from me.

              Show
              damyon Damyon Wiese added a comment - After considering it I am OK with this change. +1 from me.
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              I've had a chat to Damyon about this and had a bit of a think about how we could do this. Unfortunately it isn't as simple as just hiding the review panel is unoconv isn't installed (or is the wrong version) because students may still upload PDFs (which wouldn't need converting by unoconv).

              This means that it actually requires a per user solution, where the panel is shown/hidden on a user change depending on whether or not the user has a file submission (this is distinct from whether unoconv is installed or not). In order to achieve this we'll have to rework the current interface and code quite a bit.

              Given this is a fairly significant UI change and a fairly significant piece of work only a few days before release, I'm going to remove the "must fix" label from this issue and unassign myself.

              Some technical notes for how this could be implemented in the future:

              • Need to remove the showreview condition from this template mod/assign/templates/grading_app.mustache so that the panel html is always rendered. Instead it should be hidden using CSS (adding a class) and the grading panel can be rendered in full width mode
              • There is a hook for the feedback plugins to call in and add content to the review panel in here mod/assign/amd/src/grading_review_panel.js (getReviewPanel). The visibility of the panel needs to be tied into that logic so that if the plugin adds content then the panel is displayed, otherwise it isn't.
              • We'll need to stop the editpdf feedback plugin from generating a blank PDF if it isn't able to find a user submitted one. That is being done in mod/assign/feedback/editpdf/classes/document_services.php generate_combined_pdf_for_attempt
              • It would be nice if we were optimistic about the visibility of the panel, i.e. if it's visible and we change user we don't hide it immediately; we hide it when we know for certain there is no content.
              • Some sweet CSS3 animations wouldn't hurt
              Show
              ryanwyllie Ryan Wyllie added a comment - I've had a chat to Damyon about this and had a bit of a think about how we could do this. Unfortunately it isn't as simple as just hiding the review panel is unoconv isn't installed (or is the wrong version) because students may still upload PDFs (which wouldn't need converting by unoconv). This means that it actually requires a per user solution, where the panel is shown/hidden on a user change depending on whether or not the user has a file submission (this is distinct from whether unoconv is installed or not). In order to achieve this we'll have to rework the current interface and code quite a bit. Given this is a fairly significant UI change and a fairly significant piece of work only a few days before release, I'm going to remove the "must fix" label from this issue and unassign myself. Some technical notes for how this could be implemented in the future: Need to remove the showreview condition from this template mod/assign/templates/grading_app.mustache so that the panel html is always rendered. Instead it should be hidden using CSS (adding a class) and the grading panel can be rendered in full width mode There is a hook for the feedback plugins to call in and add content to the review panel in here mod/assign/amd/src/grading_review_panel.js (getReviewPanel). The visibility of the panel needs to be tied into that logic so that if the plugin adds content then the panel is displayed, otherwise it isn't. We'll need to stop the editpdf feedback plugin from generating a blank PDF if it isn't able to find a user submitted one. That is being done in mod/assign/feedback/editpdf/classes/document_services.php generate_combined_pdf_for_attempt It would be nice if we were optimistic about the visibility of the panel, i.e. if it's visible and we change user we don't hide it immediately; we hide it when we know for certain there is no content. Some sweet CSS3 animations wouldn't hurt
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              Damyon and I were discussing this again and we agreed that the best solution might be to have the edit pdf panel collapsible but a button so that the user can choose whether it is visible or not.

              Show
              ryanwyllie Ryan Wyllie added a comment - Damyon and I were discussing this again and we agreed that the best solution might be to have the edit pdf panel collapsible but a button so that the user can choose whether it is visible or not.
              Hide
              markn Mark Nelson added a comment -

              Doesn't make sense to have it shown at all, even if there is an option to collapse it, if it doesn't serve any purpose.

              Show
              markn Mark Nelson added a comment - Doesn't make sense to have it shown at all, even if there is an option to collapse it, if it doesn't serve any purpose.
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              Just added a patch to this issue that I've been working on. Needs more work but gives an idea where I'm going with it.

              Show
              ryanwyllie Ryan Wyllie added a comment - Just added a patch to this issue that I've been working on. Needs more work but gives an idea where I'm going with it.
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              I've pushed up a few more changes. I've also asked Barbara Ramiro to take a look at the collapse icons I'm using to see if she can improve them.

              Show
              ryanwyllie Ryan Wyllie added a comment - I've pushed up a few more changes. I've also asked Barbara Ramiro to take a look at the collapse icons I'm using to see if she can improve them.
              Hide
              barbararamiro Barbara Ramiro added a comment - - edited

              Hi Ryan,

              As discussed the other day, I came up with 2 studies for the UI and icons...

              Study1: Toggle button on the grading area (top right side)

              Toggle - Grading default Toggle - Grading expanded

              Study2: Layout buttons on the bottom right

              Grading default Grading expanded PDF expanded

              These buttons wont be visible on narrow screens.

              Show
              barbararamiro Barbara Ramiro added a comment - - edited Hi Ryan, As discussed the other day, I came up with 2 studies for the UI and icons... Study1 : Toggle button on the grading area (top right side) Toggle - Grading default Toggle - Grading expanded Study2 : Layout buttons on the bottom right Grading default Grading expanded PDF expanded These buttons wont be visible on narrow screens.
              Hide
              cfulton Charles Fulton added a comment -

              In this proposed solution, would the following scenario be valid:

              • Ghostscript is installed, unoconv is not installed
              • If a PDF is uploaded, the annotation interface appears and I can annotate it
              • If something other than a PDF is uploaded, the annotation interface does not appear

              That would maintain the pre-3.1 functionality (as I understand it). Thanks.

              Show
              cfulton Charles Fulton added a comment - In this proposed solution, would the following scenario be valid: Ghostscript is installed, unoconv is not installed If a PDF is uploaded, the annotation interface appears and I can annotate it If something other than a PDF is uploaded, the annotation interface does not appear That would maintain the pre-3.1 functionality (as I understand it). Thanks.
              Hide
              rexj Jedidiah Rex added a comment -

              FWIW, I like the buttons in Study 2 better. I think it is clearer what they are doing.

              By not being view-able on narrow screens do you mean like on a mobile screen? This may not be a deal breaker as I am not sure who would want to grade assignments on a phone.

              Show
              rexj Jedidiah Rex added a comment - FWIW, I like the buttons in Study 2 better. I think it is clearer what they are doing. By not being view-able on narrow screens do you mean like on a mobile screen? This may not be a deal breaker as I am not sure who would want to grade assignments on a phone.
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              @Charles
              I think we're moving forward with the idea of allowing the teacher to manually collapse/expand the annotation panel where they see fit, rather than attempting to show and hide that panel per user. The end result should look something like what Barbara posted screen shots of above in the "study 2" part. I think this offers the best flexibility for the teacher in the end because they can choose to give more screen realestate to the most relevant part to them.

              @Jedidiah
              Thanks, I agree. Study 2 is much clearer, in my opinion.

              Yep, the grading interface changes layout on smaller screen size. The annotation panel ends up above the grade panel (rather than side by side) so we'll have to offer a slightly different solution for narrower resolutions. Likely just having each panel individually collapsible or something.

              Show
              ryanwyllie Ryan Wyllie added a comment - @Charles I think we're moving forward with the idea of allowing the teacher to manually collapse/expand the annotation panel where they see fit, rather than attempting to show and hide that panel per user. The end result should look something like what Barbara posted screen shots of above in the "study 2" part. I think this offers the best flexibility for the teacher in the end because they can choose to give more screen realestate to the most relevant part to them. @Jedidiah Thanks, I agree. Study 2 is much clearer, in my opinion. Yep, the grading interface changes layout on smaller screen size. The annotation panel ends up above the grade panel (rather than side by side) so we'll have to offer a slightly different solution for narrower resolutions. Likely just having each panel individually collapsible or something.
              Hide
              barbararamiro Barbara Ramiro added a comment -

              Hi Ryan Wyllie,

              Feel free to cherry pick the icons on https://github.com/barbararamiro/moodle/tree/MDL-54165-master-icon

              It includes 8 icons (4 png and 4 svg):

              • default icon
              • default icon for RTL
              • expand left
              • expand right

              Cheers (" ,)

              Show
              barbararamiro Barbara Ramiro added a comment - Hi Ryan Wyllie , Feel free to cherry pick the icons on https://github.com/barbararamiro/moodle/tree/MDL-54165-master-icon It includes 8 icons (4 png and 4 svg): default icon default icon for RTL expand left expand right Cheers (" ,)
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-54165 using repository: git://github.com/ryanwyllie/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-54165 using repository: git://github.com/ryanwyllie/moodle.git Testing instructions are missing. master (0 errors / 9 warnings) [branch: MDL-54165-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/9) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Ryan,

              The code looks good I like the usability and I only saw two real problems with it.

              1 - keyboard accessibility
              When the grading panel is collapsed (mobile view) - it is still keyboard accessible (I can tab to it). It's actually worse on the non-mobile view because I can tab to the nodes in a hidden panel - but nothing gets read (because they are off-screen?).

              2 - ghostscript not installed
              When ghostscript is not installed, or set to an invalid path - the grading review panel is not supposed to display at all. The grading panel should always be full-width. With this patch the grading review panel is displayed - it contains a spinner that never stops spinning.

              Also: These style issues caught my attention:

              1. The return type for methods returning a JQuery result is actually a JQuery instance (not object).
              2. Elsewhere in the mod assignment code - dom elements are marked and searched for using data-region tags - you have used an id here (#review-panel-toggle) which is inconsistent. I used data-region because in principal I try and write my templates so they can be inserted with no knowledge of the rest of the page. id attributes are like global variables in that regard - there can only be one.
              3. These selectors:

                    [data-region="grade-actions"] button.collapse-grade-panel
                    

                Assume that the DOM element is a button - which would prevent a themer changing it to anything that is not a button in the template without also changing the javascript. Do we need that much specificity in the selector ?

              4. When chaining function calls the indent should be 8 spaces instead of 4.

              Regards - Damyon

              Show
              damyon Damyon Wiese added a comment - Thanks Ryan, The code looks good I like the usability and I only saw two real problems with it. 1 - keyboard accessibility When the grading panel is collapsed (mobile view) - it is still keyboard accessible (I can tab to it). It's actually worse on the non-mobile view because I can tab to the nodes in a hidden panel - but nothing gets read (because they are off-screen?). 2 - ghostscript not installed When ghostscript is not installed, or set to an invalid path - the grading review panel is not supposed to display at all. The grading panel should always be full-width. With this patch the grading review panel is displayed - it contains a spinner that never stops spinning. Also: These style issues caught my attention: The return type for methods returning a JQuery result is actually a JQuery instance (not object). Elsewhere in the mod assignment code - dom elements are marked and searched for using data-region tags - you have used an id here (#review-panel-toggle) which is inconsistent. I used data-region because in principal I try and write my templates so they can be inserted with no knowledge of the rest of the page. id attributes are like global variables in that regard - there can only be one. These selectors: [data-region="grade-actions"] button.collapse-grade-panel Assume that the DOM element is a button - which would prevent a themer changing it to anything that is not a button in the template without also changing the javascript. Do we need that much specificity in the selector ? When chaining function calls the indent should be 8 spaces instead of 4. Regards - Damyon
              Hide
              gaudreaj Jean-Philippe Gaudreau added a comment -

              Hi guys,

              Thanks for working on this issue! Little note: Please backport this fix to 3.1.

              Cheers!

              Show
              gaudreaj Jean-Philippe Gaudreau added a comment - Hi guys, Thanks for working on this issue! Little note: Please backport this fix to 3.1. Cheers!
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              Thanks, Damyon. I fixed up all of the points you mention.

              I also had to end up doing quite a rework of the code because I realised too much logic was crammed into grade_review_panel.js. I've split it out into the different respective modules now and have them communicating through events.

              Could you please give it another quick once over since the code has changed so much?

              Show
              ryanwyllie Ryan Wyllie added a comment - Thanks, Damyon. I fixed up all of the points you mention. I also had to end up doing quite a rework of the code because I realised too much logic was crammed into grade_review_panel.js. I've split it out into the different respective modules now and have them communicating through events. Could you please give it another quick once over since the code has changed so much?
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks with warnings.

              Checked MDL-54165 using repository: git://github.com/ryanwyllie/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-54165 using repository: git://github.com/ryanwyllie/moodle.git MOODLE_31_STABLE (0 errors / 0 warnings) [branch: MDL-54165-31 | CI Job ] master (0 errors / 9 warnings) [branch: MDL-54165-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/9) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Ryan: One small thing is that the new file grading_events.js has a copy/pasted jsdoc comment.

              Also - the tests should

              That's the only thing - everything else looks correct and works nicely. Thanks for working on this it is a good improvement.

              Sending for integration.

              Note for integrators - this is listed as a bug but I opened a backport request for it anyway since it is a ui change. IMO this is an important thing to fix for 3.1.

              Show
              damyon Damyon Wiese added a comment - Thanks Ryan: One small thing is that the new file grading_events.js has a copy/pasted jsdoc comment. Also - the tests should That's the only thing - everything else looks correct and works nicely. Thanks for working on this it is a good improvement. Sending for integration. Note for integrators - this is listed as a bug but I opened a backport request for it anyway since it is a ui change. IMO this is an important thing to fix for 3.1.
              Hide
              poltawski Dan Poltawski added a comment -

              Lets just agree that this needs to be fixed in 3.1 now, and not bother with the backport request.

              Show
              poltawski Dan Poltawski added a comment - Lets just agree that this needs to be fixed in 3.1 now, and not bother with the backport request.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              +1 for straight fixing on 3.1 (where it's really hurting)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - +1 for straight fixing on 3.1 (where it's really hurting)
              Hide
              wsam@zhaw.ch Samuel Witzig added a comment -

              +1 to fix this in 3.1 as soon as possible, since it does really hurt (agreeing 100% to Eloy's comment)

              Show
              wsam@zhaw.ch Samuel Witzig added a comment - +1 to fix this in 3.1 as soon as possible, since it does really hurt (agreeing 100% to Eloy's comment)
              Hide
              markn Mark Nelson added a comment -

              Obviously my +1 since I created the issue and linked it as a blocker for the 3.1 release - with the blocker just being removed in order to release. I knew it was going to bother people straight away.

              Show
              markn Mark Nelson added a comment - Obviously my +1 since I created the issue and linked it as a blocker for the 3.1 release - with the blocker just being removed in order to release. I knew it was going to bother people straight away.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks for your work on this Ryan. The changes look good and are definitely a worthwhile improvement.

              I have now integrated these changes, though I feel that we need a couple of follow-on issues as the UX still isn't perfect.
              Firstly, the UI is very similar when unoconv is unavailable but there is a submission, to when there is no submission. The document window shows in both cases with an empty window.

              I feel that it would be better to have some indication in that window when there is no submission, and similarly when conversion failed for whatever reason.

              I also noticed that tab order on the document window is still backwards.

              Otherwise that change looks good and I have now integrated it.

              Integrated to 31, and master only. Over to testing.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks for your work on this Ryan. The changes look good and are definitely a worthwhile improvement. I have now integrated these changes, though I feel that we need a couple of follow-on issues as the UX still isn't perfect. Firstly, the UI is very similar when unoconv is unavailable but there is a submission, to when there is no submission. The document window shows in both cases with an empty window. I feel that it would be better to have some indication in that window when there is no submission, and similarly when conversion failed for whatever reason. I also noticed that tab order on the document window is still backwards. Otherwise that change looks good and I have now integrated it. Integrated to 31, and master only. Over to testing.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Just pulled a doc fix from Ryan too and applied to 31, and master.

              Show
              dobedobedoh Andrew Nicols added a comment - Just pulled a doc fix from Ryan too and applied to 31, and master.
              Hide
              dmonllao David Monllaó added a comment -

              Thanks Ryan. It all work nicely, thanks. Tested in 31 and master.

              Show
              dmonllao David Monllaó added a comment - Thanks Ryan. It all work nicely, thanks. Tested in 31 and master.
              Hide
              ryanwyllie Ryan Wyllie added a comment -

              I've created MDL-55145 as a follow up for the UI/UX issue Andrew mentioned.

              Show
              ryanwyllie Ryan Wyllie added a comment - I've created MDL-55145 as a follow up for the UI/UX issue Andrew mentioned.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Closing this as fixed, once it's already out there, upstream. Soon the orange hordes will start enjoying with your awesome changes, many many thanks!

              Each problem that I solved became a rule,
              which served afterwards to solve other problems.
              – René Descartes

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Closing this as fixed, once it's already out there, upstream. Soon the orange hordes will start enjoying with your awesome changes, many many thanks! Each problem that I solved became a rule, which served afterwards to solve other problems. – René Descartes

                People

                • Votes:
                  27 Vote for this issue
                  Watchers:
                  33 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Jul/16