Moodle
  1. Moodle
  2. MDL-39155

User picture shown during quiz should be large version not small one

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3
    • Fix Version/s: 2.6
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1.Go to Quiz settings page and select one of the 3 options for user image.
      2.Small image on selection gives small image, none image on selection gives no image and large image on selection gives large image.
      3.Login as student and attempt the quiz.
      4.You will see the user picture being displayed according to your selection.

      Show
      1.Go to Quiz settings page and select one of the 3 options for user image. 2.Small image on selection gives small image, none image on selection gives no image and large image on selection gives large image. 3.Login as student and attempt the quiz. 4.You will see the user picture being displayed according to your selection.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      49751

      Description

      When the option is set to display user picture during quiz, it displays the tiny thumbnail version of the image. This is not suitable for its intended use. An invigilator would need to get so close to the display for the image to be recognisable. Could the large version of the picture be used instead? Or at least an option (e.g. Display user picture: No/Large/Small).

        Activity

        Hide
        Tim Hunt added a comment -

        Good idea. I think changing it to a No/Small/Large setting is the way to go.

        Show
        Tim Hunt added a comment - Good idea. I think changing it to a No/Small/Large setting is the way to go.
        Show
        Tim Hunt added a comment - Reviewing https://github.com/jacks92/moodle/compare/master...MDL-39155 .
        Hide
        Tim Hunt added a comment -

        Your approach to fixing this is basically correct. Well done.

        There is one significant problem

        1. In mod/quiz/attemptlib.php. You have copied and pasted a chunk of code, so now there are two coplies. Don't do things like that. You need to find a better way to write this bit.

        Then there are some minor things that show this code is not really finished and ready for review:

        2. you have a commit relating to another bug in https://github.com/jacks92/moodle/compare/master...MDL-39155. The branch needs to be cleaned up so there is a single commit.

        3.There are blank PHPdoc comments in mod/quiz/locallib.php. Those need to be fixed.

        4. There is incorrect whitespace and coding style in several places.

        Show
        Tim Hunt added a comment - Your approach to fixing this is basically correct. Well done. There is one significant problem 1. In mod/quiz/attemptlib.php. You have copied and pasted a chunk of code, so now there are two coplies. Don't do things like that. You need to find a better way to write this bit. Then there are some minor things that show this code is not really finished and ready for review: 2. you have a commit relating to another bug in https://github.com/jacks92/moodle/compare/master...MDL-39155 . The branch needs to be cleaned up so there is a single commit. 3.There are blank PHPdoc comments in mod/quiz/locallib.php. Those need to be fixed. 4. There is incorrect whitespace and coding style in several places.
        Hide
        Jayesh Anandani added a comment -

        1. Sir there are 3 possibilities so i have taken 3 if loops for them.And all contain different code.
        There is a slight difference between 2 and 3rd loop.In 3rd loop the size parameter is also being passed while it is not so in case of 2nd loop.So this was best possible way i could have done that.Any alternate way are most welcome.

        2,3,4: Solved them.Checked them through codechecker,inserted comments and finally my link has only one commit.

        Show
        Jayesh Anandani added a comment - 1. Sir there are 3 possibilities so i have taken 3 if loops for them.And all contain different code. There is a slight difference between 2 and 3rd loop.In 3rd loop the size parameter is also being passed while it is not so in case of 2nd loop.So this was best possible way i could have done that.Any alternate way are most welcome. 2,3,4: Solved them.Checked them through codechecker,inserted comments and finally my link has only one commit.
        Hide
        Tim Hunt added a comment -

        Much less duplication:

        $user = $DB->get_record('user', array('id' => $this->attemptobj->get_userid()));
        $userpicture = new user_picture($user);
        $userpicture->courseid = $this->attemptobj->get_courseid();
        if ($this->attemptobj->get_quiz()->showuserpicture==QUIZ_SHOWIMAGE_LARGE) {
            $userpicture->size = true;
        }
        return $userpicture;
        

        There are still whitespace errors, even if code-checker does not notice. I would put a space either side of ==. In the lang file, all the other lines have a space either side of =. You should not have a space between the PHPdoc comment and the define. In the form, you should indent the lines starting QUIZ_ to make it clear they continue the line before.

        Show
        Tim Hunt added a comment - Much less duplication: $user = $DB->get_record('user', array('id' => $ this ->attemptobj->get_userid())); $userpicture = new user_picture($user); $userpicture->courseid = $ this ->attemptobj->get_courseid(); if ($ this ->attemptobj->get_quiz()->showuserpicture==QUIZ_SHOWIMAGE_LARGE) { $userpicture->size = true ; } return $userpicture; There are still whitespace errors, even if code-checker does not notice. I would put a space either side of ==. In the lang file, all the other lines have a space either side of =. You should not have a space between the PHPdoc comment and the define. In the form, you should indent the lines starting QUIZ_ to make it clear they continue the line before.
        Hide
        Jayesh Anandani added a comment -

        Would make the changes and update it.

        Show
        Jayesh Anandani added a comment - Would make the changes and update it.
        Hide
        Jayesh Anandani added a comment -

        All the errors along with code duplication checked and updated.

        Show
        Jayesh Anandani added a comment - All the errors along with code duplication checked and updated.
        Hide
        Tim Hunt added a comment -

        Well, some of the whitespace errors fixed. Surely it is not that hard to get the whitespace right. Look at other examples of code in the same files.

        Show
        Tim Hunt added a comment - Well, some of the whitespace errors fixed. Surely it is not that hard to get the whitespace right. Look at other examples of code in the same files.
        Hide
        Jayesh Anandani added a comment -

        Except that space, are the other indentations and code properly formated?
        I took into account mistakes pointed and again checked them with codechecker.

        Show
        Jayesh Anandani added a comment - Except that space, are the other indentations and code properly formated? I took into account mistakes pointed and again checked them with codechecker.
        Hide
        Jayesh Anandani added a comment -

        Well, yes sir those whitespace aren't to hard to get.I will try to minimize the error as much as possible.

        Show
        Jayesh Anandani added a comment - Well, yes sir those whitespace aren't to hard to get.I will try to minimize the error as much as possible.
        Hide
        Jayesh Anandani added a comment -

        Patch modified and testing instructions included.

        Show
        Jayesh Anandani added a comment - Patch modified and testing instructions included.
        Hide
        Tim Hunt added a comment -

        I have tested and it works.

        I just spotted two more issues:

        1. We need a corresponding chagne in settings.php

        2. " not ' in the lang file.

        Show
        Tim Hunt added a comment - I have tested and it works. I just spotted two more issues: 1. We need a corresponding chagne in settings.php 2. " not ' in the lang file.
        Hide
        Jayesh Anandani added a comment -

        1. Changes done to settings.php

        2. " replaced by ' in lang file.

        3. Provided space between == .

        Show
        Jayesh Anandani added a comment - 1. Changes done to settings.php 2. " replaced by ' in lang file. 3. Provided space between == .
        Hide
        Tim Hunt added a comment -

        Yay! we got there. Submitting for integration now.

        Show
        Tim Hunt added a comment - Yay! we got there. Submitting for integration now.
        Hide
        Ian McNaught added a comment -

        Thanks all for addressing this issue. Having been a Blackboard guy for 6 years, it is so refreshing to see how quickly even trivial things like this get addressed in the Moodle community.

        Show
        Ian McNaught added a comment - Thanks all for addressing this issue. Having been a Blackboard guy for 6 years, it is so refreshing to see how quickly even trivial things like this get addressed in the Moodle community.
        Hide
        Dan Poltawski added a comment -

        Thanks Jayesh!

        I've integrated this to master

        Show
        Dan Poltawski added a comment - Thanks Jayesh! I've integrated this to master
        Hide
        Jason Fowler added a comment -

        Thanks Jayesh and Tim, great work!

        Show
        Jason Fowler added a comment - Thanks Jayesh and Tim, great work!
        Hide
        Marina Glancy added a comment -

        Thanks for your awesome work! This has now become a part of Moodle.

        Closing as fixed!

        Show
        Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!
        Hide
        Mary Cooch added a comment -

        Added a QA test for this here MDLQA-5737 ready for inclusion in 2.6 testing.

        Show
        Mary Cooch added a comment - Added a QA test for this here MDLQA-5737 ready for inclusion in 2.6 testing.
        Hide
        Mary Cooch added a comment -

        Removing docs_required label as I mentioned it here http://docs.moodle.org/26/en/Quiz_settings

        Show
        Mary Cooch added a comment - Removing docs_required label as I mentioned it here http://docs.moodle.org/26/en/Quiz_settings

          People

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

            Dates

            • Created:
              Updated:
              Resolved: