From: David Mudrak <david.mudrak@gmail.com>
Date: Mon, 4 Jan 2010 18:02:05 +0000 (+0000)
Subject: Refactoring some critical parts
X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=3dc78e5b04e42dfc93a1b1a51d391bf537370cbe;p=moodle.git

Refactoring some critical parts

Critical issue fixed here: after recent refactorings, I called
get_users_with_capability() inside the loop in another loop! (oops :-)
This was used in my very first code using the renderers. I have learnt
I should follow the core approach (as suggested by Tim) to prepare a
data object and pass it to the renderer. The fact the renderer called
a workshop method indicated something was really wrong...

WIP
---

diff --git a/mod/workshop/allocation/manual/allocator.php b/mod/workshop/allocation/manual/allocator.php
index 3fc6156c4a..739615c1cf 100644
--- a/mod/workshop/allocation/manual/allocator.php
+++ b/mod/workshop/allocation/manual/allocator.php
@@ -129,9 +129,7 @@ class workshop_manual_allocator implements workshop_allocator {
      * Prints user interface - current allocation and a form to edit it
      */
     public function ui() {
-        global $PAGE;
-        global $CFG;    // bacause we include other libs here
-        global $OUTPUT;
+        global $PAGE, $OUTPUT;
 
         $hlauthorid     = -1;           // highlight this author
         $hlreviewerid   = -1;           // highlight this reviewer
@@ -225,11 +223,21 @@ class workshop_manual_allocator implements workshop_allocator {
             }
         }
 
-        // We have all data. Let it pass to the renderer and return the output
-        // Here, we do not use neither the core renderer nor the workshop one but use an own one
-        require_once(dirname(__FILE__) . '/renderer.php');
+        // we have all data, let us pass it to the renderer and return the output
+        $wsoutput = $PAGE->theme->get_renderer('mod_workshop', $PAGE);
         $uioutput = $PAGE->theme->get_renderer('workshopallocation_manual', $PAGE);
-        return $uioutput->display_allocations($this->workshop, $peers, $hlauthorid, $hlreviewerid, $msg);
+        // prepare data to be displayed
+        $data                    = new stdClass();
+        $data->wsoutput          = $wsoutput;
+        $data->peers             = $peers;
+        $data->authors           = $this->workshop->get_potential_authors();
+        $data->reviewers         = $this->workshop->get_potential_reviewers();
+        $data->hlauthorid        = $hlauthorid;
+        $data->hlreviewerid      = $hlreviewerid;
+        $data->msg               = $msg;
+        $data->useselfassessment = $this->workshop->useselfassessment;
+
+        return $uioutput->display_allocations($data);
     }
 
 }
diff --git a/mod/workshop/allocation/manual/renderer.php b/mod/workshop/allocation/manual/renderer.php
index 645f53dae6..8d408f39d5 100644
--- a/mod/workshop/allocation/manual/renderer.php
+++ b/mod/workshop/allocation/manual/renderer.php
@@ -51,14 +51,18 @@ class moodle_workshopallocation_manual_renderer extends moodle_renderer_base  {
     /**
      * Display the table of all current allocations and widgets to modify them
      *
-     * @param workshop $workshop workshop API instance
-     * @param array $peers prepared array of all allocations
-     * @param int $hlauthorid highlight this author
-     * @param int $hlreviewerid highlight this reviewer
-     * @param stdClass $msg message to display
+     * @param stdClass $data to be displayed - see the top of the function for the list of needed properties
      * @return string html code
      */
-    public function display_allocations(workshop $workshop, $peers, $hlauthorid=null, $hlreviewerid=null, $msg=null) {
+    public function display_allocations(stdClass $data) {
+        $wsoutput           = $data->wsoutput;          // moodle_mod_workshop_renderer
+        $peers              = $data->peers;             // array prepared array of all allocations data
+        $authors            = $data->authors;           // array submission authors
+        $reviewers          = $data->reviewers;         // array potential submission reviewers
+        $hlauthorid         = $data->hlauthorid;        // int id of the author to highlight
+        $hlreviewerid       = $data->hlreviewerid;      // int id of the reviewer to highlight
+        $useselfassessment  = $data->useselfassessment; // bool is the self-assessment allowed in this workshop?
+        $msg                = $data->msg;               // stdClass message to display
 
         $wsoutput = $this->page->theme->get_renderer('mod_workshop', $this->page);
         if (empty($peers)) {
@@ -75,9 +79,9 @@ class moodle_workshopallocation_manual_renderer extends moodle_renderer_base  {
         $table->data        = array();
         foreach ($peers as $user) {
             $row = array();
-            $row[] = $this->reviewers_of_participant($user, $workshop, $peers);
-            $row[] = $this->participant($user, $workshop);
-            $row[] = $this->reviewees_of_participant($user, $workshop, $peers);
+            $row[] = $this->reviewers_of_participant($user, $peers, $reviewers, $useselfassessment);
+            $row[] = $this->participant($user);
+            $row[] = $this->reviewees_of_participant($user, $peers, $authors, $useselfassessment);
             $thisrowclasses = array();
             if ($user->id == $hlauthorid) {
                 $thisrowclasses[] = 'highlightreviewedby';
@@ -99,22 +103,18 @@ class moodle_workshopallocation_manual_renderer extends moodle_renderer_base  {
      * @param workshop API
      * @return string HTML code
      */
-    protected function participant(stdClass $user, workshop $workshop) {
+    protected function participant(stdClass $user) {
         $o  = $this->output->user_picture($user, $this->page->course->id);
         $o .= fullname($user);
         $o .= $this->output->container_start(array('submission'));
         if (is_null($user->submissionid)) {
-            $o .= $this->output->output_tag('span', array('class' => 'info'), get_string('nosubmissionfound', 'workshop'));
+            $o .= $this->output->container(get_string('nosubmissionfound', 'workshop'), 'info');
         } else {
-            $submlink = new html_link();
-            $submlink->url = new moodle_url($workshop->submission_url(), array('id' => $user->submissionid));
-            $submlink->text = format_string($user->submissiontitle);
-            $submlink->set_classes('title');
-            $o .= $this->output->link($submlink);
+            $o .= $this->output->container(format_string($user->submissiontitle), 'title');
             if (is_null($user->submissiongrade)) {
                 $o .= $this->output->container(get_string('nogradeyet', 'workshop'), array('grade', 'missing'));
             } else {
-                $o .= $this->output->container(s($user->submissiongrade), array('grade')); // TODO calculate grade
+                $o .= $this->output->container(get_string('alreadygraded', 'workshop'), array('grade', 'missing'));
             }
         }
         $o .= $this->output->container_end();
@@ -124,29 +124,31 @@ class moodle_workshopallocation_manual_renderer extends moodle_renderer_base  {
     /**
      * Returns information about the current reviewers of the given participant and a selector do add new one
      *
-     * @param stdClass $user         participant data
-     * @param workshop $workshop workshop record
-     * @param array $peers           objects with properties to display picture and fullname
+     * @param stdClass $user          participant data
+     * @param array $peers            objects with properties to display picture and fullname
+     * @param array $reviewers        potential reviewers
+     * @param bool $useselfassessment shall a user be offered as a reviewer of him/herself
      * @return string html code
      */
-    protected function reviewers_of_participant(stdClass $user, workshop $workshop, $peers) {
+    protected function reviewers_of_participant(stdClass $user, array $peers, array $reviewers, $useselfassessment) {
         $o = '';
         if (is_null($user->submissionid)) {
-            $o .= $this->output->output_tag('span', array('class' => 'info'), get_string('nothingtoreview', 'workshop'));
+            $o .= $this->output->container(get_string('nothingtoreview', 'workshop'), 'info');
         } else {
             $exclude = array();
-            if (!$workshop->useselfassessment) {
+            if (! $useselfassessment) {
                 $exclude[$user->id] = true;
             }
             // todo add an option to exclude users without own submission
-            // todo nice to have number of current allocations for every user plus ordering by it
-            $handler = new moodle_url($this->page->url, array('mode' => 'new', 'of' => $user->id, 'sesskey' => sesskey()));
-            $options = $this->users_to_menu_options($workshop->get_peer_reviewers(), $exclude);
-            $select = html_select::make_popup_form($handler, 'by', $options, 'addreviewof' . $user->id, '',
-                get_string('addreviewer', 'workshopallocation_manual'));
-            $select->nothinglabel = get_string('chooseuser', 'workshop');
-            $select->set_label(get_string('addreviewer', 'workshopallocation_manual'), $select->id);
-            $o .= $this->output->select($select);
+            $options = $this->users_to_menu_options($reviewers, $exclude);
+            if ($options) {
+                $handler = new moodle_url($this->page->url, array('mode' => 'new', 'of' => $user->id, 'sesskey' => sesskey()));
+                $select = html_select::make_popup_form($handler, 'by', $options, 'addreviewof' . $user->id, '',
+                    get_string('addreviewer', 'workshopallocation_manual'));
+                $select->nothinglabel = get_string('chooseuser', 'workshop');
+                $select->set_label(get_string('addreviewer', 'workshopallocation_manual'), $select->id);
+                $o .= $this->output->select($select);
+            }
         }
         $o .= $this->output->output_start_tag('ul', array());
         foreach ($user->reviewedby as $reviewerid => $assessmentid) {
@@ -171,30 +173,34 @@ class moodle_workshopallocation_manual_renderer extends moodle_renderer_base  {
     /**
      * Returns information about the current reviewees of the given participant and a selector do add new one
      *
-     * @param stdClass $user         participant data
-     * @param workshop $workshop workshop record
-     * @param array $peers           objects with properties to display picture and fullname
+     * @param stdClass $user          participant data
+     * @param array $peers            objects with properties to display picture and fullname
+     * @param array $authors          potential authors to be reviewed
+     * @param bool $useselfassessment shall a user be offered as a reviewer of him/herself
      * @return string html code
      */
-    protected function reviewees_of_participant(stdClass $user, workshop $workshop, $peers) {
+    protected function reviewees_of_participant(stdClass $user, array $peers, array $authors, $useselfassessment) {
         $o = '';
         if (is_null($user->submissionid)) {
             $o .= $this->output->container(get_string('withoutsubmission', 'workshop'), 'info');
         }
         $exclude = array();
-        if (!$workshop->useselfassessment) {
+        if (! $useselfassessment) {
             $exclude[$user->id] = true;
             $o .= $this->output->container(get_string('selfassessmentdisabled', 'workshop'), 'info');
         }
         // todo add an option to exclude users without own submission
-        // todo nice to have number of current allocations for every user plus ordering by it
-        $handler = new moodle_url($this->page->url, array('mode' => 'new', 'by' => $user->id, 'sesskey' => sesskey()));
-        $options = $this->users_to_menu_options($workshop->get_peer_authors(), $exclude);
-        $select = html_select::make_popup_form($handler, 'of', $options, 'addreviewby' . $user->id, '',
-            get_string('addreviewee', 'workshopallocation_manual'));
-        $select->nothinglabel = get_string('chooseuser', 'workshop');
-        $select->set_label(get_string('addreviewee', 'workshopallocation_manual'), $select->id);
-        $o .= $this->output->select($select);
+        $options = $this->users_to_menu_options($authors, $exclude);
+        if ($options) {
+            $handler = new moodle_url($this->page->url, array('mode' => 'new', 'by' => $user->id, 'sesskey' => sesskey()));
+            $select = html_select::make_popup_form($handler, 'of', $options, 'addreviewby' . $user->id, '',
+                                                        get_string('addreviewee', 'workshopallocation_manual'));
+            $select->nothinglabel = get_string('chooseuser', 'workshop');
+            $select->set_label(get_string('addreviewee', 'workshopallocation_manual'), $select->id);
+            $o .= $this->output->select($select);
+        } else {
+            $o .= $this->output->container(get_string('nothingtoreview', 'workshop'), 'info');
+        }
         $o .= $this->output->output_start_tag('ul', array());
         foreach ($user->reviewerof as $authorid => $assessmentid) {
             $o .= $this->output->output_start_tag('li', array());
diff --git a/mod/workshop/allocation/random/allocator.php b/mod/workshop/allocation/random/allocator.php
index 07ec9f670b..a6f3454474 100644
--- a/mod/workshop/allocation/random/allocator.php
+++ b/mod/workshop/allocation/random/allocator.php
@@ -80,11 +80,11 @@ class workshop_random_allocator implements workshop_allocator {
             $musthavesubmission = empty($assesswosubmission);
             $addselfassessment  = optional_param('addselfassessment', false, PARAM_INT); // may be frozen in the form
 
-            $authors            = $this->workshop->get_peer_authors();
+            $authors            = $this->workshop->get_potential_authors();
             $authors            = $this->workshop->get_grouped($authors);
-            $reviewers          = $this->workshop->get_peer_reviewers($musthavesubmission);
+            $reviewers          = $this->workshop->get_potential_reviewers($musthavesubmission);
             $reviewers          = $this->workshop->get_grouped($reviewers);
-            $assessments        = $this->workshop->get_assessments();
+            $assessments        = $this->workshop->get_all_assessments();
 
             $newallocations     = array();      // array of array(reviewer => reviewee)
 
@@ -176,6 +176,8 @@ class workshop_random_allocator implements workshop_allocator {
         ob_end_clean();
         $out .= $OUTPUT->container_end();
 
+        $out .= $OUTPUT->heading(get_string('stats', 'workshopallocation_random'));
+        // TODO
         return $out;
     }
 
@@ -185,9 +187,9 @@ class workshop_random_allocator implements workshop_allocator {
      * If the submission has already been allocated, it is skipped. If the author is not found among
      * reviewers, the submission is not assigned.
      *
-     * @param array $authors as returned by {@see workshop::get_peer_authors_by_group()}
-     * @param array $reviewers as returned by {@see workshop::get_peer_reviewers_by_group()}
-     * @param array $assessments as returned by {@see workshop::get_assessments()}
+     * @param array $authors grouped of {@see workshop::get_potential_authors()}
+     * @param array $reviewers grouped by {@see workshop::get_potential_reviewers()}
+     * @param array $assessments as returned by {@see workshop::get_all_assessments()}
      * @return array of new allocations to be created, array of array(reviewerid => authorid)
      */
     protected function self_allocation($authors=array(), $reviewers=array(), $assessments=array()) {
@@ -223,12 +225,12 @@ class workshop_random_allocator implements workshop_allocator {
      * @param array $datareviewers  reviewers by group, group [0] contains all reviewers
      * @return bool
      */
-    protected function add_new_allocations($newallocations, $dataauthors, $datareviewers) {
+    protected function add_new_allocations(array $newallocations, array $dataauthors, array $datareviewers) {
         global $DB;
 
         $newallocations = $this->get_unique_allocations($newallocations);
         $authorids      = $this->get_author_ids($newallocations);
-        $submissions    = $this->workshop->get_submission_by_author($authorids);
+        $submissions    = $this->workshop->get_submissions($authorids, false);
         $submissions    = $this->index_submissions_by_authors($submissions);
         foreach ($newallocations as $newallocation) {
             list($reviewerid, $authorid) = each($newallocation);
@@ -485,7 +487,7 @@ class workshop_random_allocator implements workshop_allocator {
     /**
      * Extracts the information about reviews from the authors' and reviewers' perspectives
      *
-     * @param array $assessments array of assessments as returned by {@link workshop::get_assessments()}
+     * @param array $assessments array of assessments as returned by {@link workshop::get_all_assessments()}
      * @return array of two arrays
      */
     protected function convert_assessments_to_links($assessments) {
diff --git a/mod/workshop/allocation/random/lang/en_utf8/workshopallocation_random.php b/mod/workshop/allocation/random/lang/en_utf8/workshopallocation_random.php
index 07420afcf7..b3f51486e2 100644
--- a/mod/workshop/allocation/random/lang/en_utf8/workshopallocation_random.php
+++ b/mod/workshop/allocation/random/lang/en_utf8/workshopallocation_random.php
@@ -40,3 +40,4 @@ $string['numperreviewer'] = 'per reviewer';
 $string['pluginname'] = 'Random allocation';
 $string['randomallocationdone'] = 'Random allocation done';
 $string['removecurrentallocations'] = 'Remove current allocations';
+$string['stats'] = 'Current allocation statistics';
diff --git a/mod/workshop/develtools.php b/mod/workshop/develtools.php
index c0725355ba..12a3589126 100644
--- a/mod/workshop/develtools.php
+++ b/mod/workshop/develtools.php
@@ -53,8 +53,8 @@ $wsoutput = $PAGE->theme->get_renderer('mod_workshop', $PAGE);
 
 switch ($tool) {
 case 'mksubmissions':
-    $authors                = $workshop->get_peer_authors(false);
-    $authorswithsubmission  = $workshop->get_peer_authors(true);
+    $authors                = $workshop->get_potential_authors(false);
+    $authorswithsubmission  = $workshop->get_potential_authors(true);
     $authors                = array_diff_key($authors, $authorswithsubmission);
     echo $OUTPUT->header();
     $c = 0; // counter
diff --git a/mod/workshop/lang/en_utf8/workshop.php b/mod/workshop/lang/en_utf8/workshop.php
index 2cfd32964b..94e1cc7837 100644
--- a/mod/workshop/lang/en_utf8/workshop.php
+++ b/mod/workshop/lang/en_utf8/workshop.php
@@ -33,7 +33,7 @@ $string[''] = '';
 $string[''] = '';
 $string[''] = '';
 $string[''] = '';
-$string[''] = '';
+$string['someuserswosubmission'] = 'There are some users who have not submitted yet';
 $string['accesscontrol'] = 'Access control';
 $string['agreeassessments'] = 'Assessments must be agreed';
 $string['agreeassessmentsdesc'] = 'Authors may comment assessments of their work and agree/disagree with it';
@@ -97,7 +97,9 @@ $string['modulename'] = 'Workshop';
 $string['nattachments'] = 'Maximum number of submission attachments';
 $string['nexassessments'] = 'Number of required assessments of examples';
 $string['nogradeyet'] = 'No grade yet';
+$string['alreadygraded'] = 'Already graded';
 $string['nosubmissionfound'] = 'No submission found for this user';
+$string['noyoursubmission'] = 'You have not submitted your work yet';
 $string['nosubmissions'] = 'No submissions yet in this workshop';
 $string['nothingtoreview'] = 'Nothing to review';
 $string['noworkshops'] = 'There are no workshops in this course';
@@ -154,7 +156,7 @@ $string['userdatecreated'] = 'submitted on <span>$a</span>';
 $string['userdatemodified'] = 'modified on <span>$a</span>';
 $string['useselfassessmentdesc'] = 'Users perform self assessment of their own work';
 $string['useselfassessment'] = 'Use self assessment';
-$string['withoutsubmission'] = 'Warning - reviewer without own submission';
+$string['withoutsubmission'] = 'Reviewer without own submission';
 $string['workshopadministration'] = 'Workshop administration';
 $string['workshopfeatures'] = 'Workshop features';
 $string['workshopname'] = 'Workshop name';
diff --git a/mod/workshop/locallib.php b/mod/workshop/locallib.php
index eaae461537..cffdd7ba20 100644
--- a/mod/workshop/locallib.php
+++ b/mod/workshop/locallib.php
@@ -136,15 +136,12 @@ class workshop {
      * @param bool $musthavesubmission If true, return only users who have already submitted. All possible authors otherwise.
      * @return array array[userid] => stdClass{->id ->lastname ->firstname}
      */
-    public function get_peer_authors($musthavesubmission=true) {
-
+    public function get_potential_authors($musthavesubmission=true) {
         $users = get_users_by_capability($this->context, 'mod/workshop:submit',
                     'u.id, u.lastname, u.firstname', 'u.lastname,u.firstname', '', '', '', '', false, false, true);
-
         if ($musthavesubmission) {
             $users = array_intersect_key($users, $this->users_with_submission(array_keys($users)));
         }
-
         return $users;
     }
 
@@ -156,17 +153,13 @@ class workshop {
      * @param bool $musthavesubmission If true, return only users who have already submitted. All possible users otherwise.
      * @return array array[userid] => stdClass{->id ->lastname ->firstname}
      */
-    public function get_peer_reviewers($musthavesubmission=false) {
-        global $DB;
-
+    public function get_potential_reviewers($musthavesubmission=false) {
         $users = get_users_by_capability($this->context, 'mod/workshop:peerassess',
                     'u.id, u.lastname, u.firstname', 'u.lastname,u.firstname', '', '', '', '', false, false, true);
-
         if ($musthavesubmission) {
             // users without their own submission can not be reviewers
             $users = array_intersect_key($users, $this->users_with_submission(array_keys($users)));
         }
-
         return $users;
     }
 
@@ -217,14 +210,14 @@ class workshop {
     /**
      * Returns submissions from this workshop
      *
-     * Fetches no-big-text data from {workshop_submissions} and adds some useful information from other
-     * tables.
+     * Fetches data from {workshop_submissions} and adds some useful information from other
+     * tables. Does not return textual fields to prevent possible memory lack issues.
      *
      * @param mixed $userid int|array|'all' If set to [array of] integer, return submission[s] of the given user[s] only
      * @param mixed $examples false|true|'all' Only regular submissions, only examples, all submissions
      * @return stdClass moodle_recordset
      */
-    public function get_submissions_recordset($userid='all', $examples=false) {
+    public function get_submissions($userid='all', $examples=false) {
         global $DB;
 
         $sql = 'SELECT s.id, s.workshopid, s.example, s.userid, s.timecreated, s.timemodified,
@@ -245,7 +238,6 @@ class workshop {
         } else {
             throw new coding_exception('Illegal parameter value: $examples may be false|true|"all"');
         }
-        $sql .= ' ORDER BY u.lastname, u.firstname';
 
         if ('all' === $userid) {
             // no additional conditions
@@ -257,8 +249,9 @@ class workshop {
             $sql .= ' AND userid = :userid';
             $params['userid'] = $userid;
         }
+        $sql .= ' ORDER BY u.lastname, u.firstname';
 
-        return $DB->get_recordset_sql($sql, $params);
+        return $DB->get_records_sql($sql, $params);
     }
 
     /**
@@ -281,14 +274,10 @@ class workshop {
     }
 
     /**
-     * Returns a submission submitted by the given author or authors.
-     *
-     * If the single one submission is requested, returns the first found record including text fields.
-     * If multiple records are requested, uses {@link self::get_submissions_recordset()}.
-     * Does not return example submissions.
+     * Returns a submission submitted by the given author
      *
-     * @param mixed $id integer|array author ID or IDs
-     * @return mixed false if not found, stdClass if $id is int, array if $id is array
+     * @param int $id author id
+     * @return stdClass|false
      */
     public function get_submission_by_author($id) {
         global $DB;
@@ -296,40 +285,30 @@ class workshop {
         if (empty($id)) {
             return false;
         }
-        if (is_array($id)) {
-            $rs = $this->get_submissions_recordset($id, false);
-            $submissions = array();
-            foreach ($rs as $submission) {
-                $submissions[$submission->id] = $submission;
-            }
-            $rs->close();
-            return $submissions;
-        } else {
-            $sql = 'SELECT s.*,
-                           u.lastname AS authorlastname, u.firstname AS authorfirstname, u.id AS authorid,
-                           u.picture AS authorpicture, u.imagealt AS authorimagealt
-                      FROM {workshop_submissions} s
-                INNER JOIN {user} u ON (s.userid = u.id)
-                     WHERE s.example = 0 AND s.workshopid = :workshopid AND s.userid = :userid';
-            $params = array('workshopid' => $this->id, 'userid' => $id);
-            return $DB->get_record_sql($sql, $params);
-        }
+        $sql = 'SELECT s.*,
+                       u.lastname AS authorlastname, u.firstname AS authorfirstname, u.id AS authorid,
+                       u.picture AS authorpicture, u.imagealt AS authorimagealt
+                  FROM {workshop_submissions} s
+            INNER JOIN {user} u ON (s.userid = u.id)
+                 WHERE s.example = 0 AND s.workshopid = :workshopid AND s.userid = :userid';
+        $params = array('workshopid' => $this->id, 'userid' => $id);
+        return $DB->get_record_sql($sql, $params);
     }
 
     /**
-     * Returns the list of assessments with some data added
+     * Returns the list of all assessments in the workshop with some data added
      *
      * Fetches data from {workshop_assessments} and adds some useful information from other
-     * tables.
-      *
-     * @param mixed $reviewerid 'all'|int|array User ID of the reviewer
-     * @param mixed $id         'all'|int Assessment ID
-     * @return stdClass moodle_recordset
+     * tables. The returned object does not contain textual fields (ie comments) to prevent memory
+     * lack issues.
+     *
+     * @return array [assessmentid] => assessment stdClass
      */
-    public function get_assessments_recordset($reviewerid='all', $id='all') {
+    public function get_all_assessments() {
         global $DB;
 
-        $sql = 'SELECT a.*,
+        $sql = 'SELECT a.id, a.submissionid, a.userid, a.timecreated, a.timemodified, a.timeagreed,
+                       a.grade, a.gradinggrade, a.gradinggradeover, a.gradinggradeoverby,
                        reviewer.id AS reviewerid,reviewer.firstname AS reviewerfirstname,reviewer.lastname as reviewerlastname,
                        s.title,
                        author.id AS authorid, author.firstname AS authorfirstname,author.lastname as authorlastname
@@ -337,95 +316,57 @@ class workshop {
             INNER JOIN {user} reviewer ON (a.userid = reviewer.id)
             INNER JOIN {workshop_submissions} s ON (a.submissionid = s.id)
             INNER JOIN {user} author ON (s.userid = author.id)
-                 WHERE s.workshopid = :workshopid';
+                 WHERE s.workshopid = :workshopid AND s.example = 0
+              ORDER BY reviewer.lastname, reviewer.firstname';
         $params = array('workshopid' => $this->id);
 
-        if ('all' === $reviewerid) {
-            // no additional conditions
-        } elseif (is_array($reviewerid)) {
-            list($usql, $uparams) = $DB->get_in_or_equal($reviewerid, SQL_PARAMS_NAMED);
-            $sql .= " AND reviewer.id $usql";
-            $params = array_merge($params, $uparams);
-        } else {
-            $sql .= ' AND reviewer.id = :reviewerid';
-            $params['reviewerid'] = $reviewerid;
-        }
-
-        if ('all' === $id) {
-            // no additional conditions
-        } else {
-            $sql .= ' AND a.id = :assessmentid';
-            $params['assessmentid'] = $id;
-        }
-
-        return $DB->get_recordset_sql($sql, $params);
-    }
-
-    /**
-     * Returns the list of assessments with some data added
-     *
-     * Fetches data from {workshop_assessments} and adds some useful information from other
-     * tables. The returned objects are lightweight version of those returned by get_assessments_recordset(),
-     * mainly they do not contain text fields.
-     *
-     * @param mixed $reviewerid 'all'|int|array User ID of the reviewer
-     * @param mixed $id         'all'|int Assessment ID
-     * @return array [assessmentid] => assessment stdClass
-     * @see workshop::get_assessments_recordset() for the structure of returned objects
-     */
-    public function get_assessments($reviewerid='all', $id='all') {
-        $rs = $this->get_assessments_recordset($reviewerid, $id);
-        $assessments = array();
-        foreach ($rs as $assessment) {
-            // copy selected properties into the array to be returned. This is here mainly in order not
-            // to include text comments.
-            $assessments[$assessment->id] = new stdClass();
-            foreach ($assessment as $property => $value) {
-                if (in_array($property, array('id', 'submissionid', 'userid', 'timecreated', 'timemodified',
-                        'timeagreed', 'grade', 'gradinggrade', 'gradinggradeover', 'gradinggradeoverby',
-                        'reviewerid', 'reviewerfirstname', 'reviewerlastname', 'title', 'authorid',
-                        'authorfirstname', 'authorlastname'))) {
-                    $assessments[$assessment->id]->{$property} = $value;
-                }
-            }
-        }
-        $rs->close();
-        return $assessments;
+        return $DB->get_records_sql($sql, $params);
     }
 
     /**
-     * Get the information about the given assessment
+     * Get the complete information about the given assessment
      *
      * @param int $id Assessment ID
-     * @see workshop::get_assessments_recordset() for the structure of data returned
      * @return mixed false if not found, stdClass otherwise
      */
     public function get_assessment_by_id($id) {
-        $rs         = $this->get_assessments_recordset('all', $id);
-        $assessment = $rs->current();
-        $rs->close();
-        if (empty($assessment->id)) {
-            return false;
-        } else {
-            return $assessment;
-        }
+        global $DB;
+
+        $sql = 'SELECT a.*,
+                       reviewer.id AS reviewerid,reviewer.firstname AS reviewerfirstname,reviewer.lastname as reviewerlastname,
+                       s.title,
+                       author.id AS authorid, author.firstname AS authorfirstname,author.lastname as authorlastname
+                  FROM {workshop_assessments} a
+            INNER JOIN {user} reviewer ON (a.userid = reviewer.id)
+            INNER JOIN {workshop_submissions} s ON (a.submissionid = s.id)
+            INNER JOIN {user} author ON (s.userid = author.id)
+                 WHERE a.id = :id AND s.workshopid = :workshopid';
+        $params = array('id' => $id, 'workshopid' => $this->id);
+
+        return $DB->get_record_sql($sql, $params, MUST_EXIST);
     }
 
     /**
-     * Get the information about all assessments assigned to the given reviewer
+     * Get the complete information about all assessments allocated to the given reviewer
      *
-     * @param int $id Reviewer ID
-     * @see workshop::get_assessments_recordset() for the structure of data returned
-     * @return array array of objects
+     * @param int $userid reviewer id
+     * @return array
      */
-    public function get_assessments_by_reviewer($id) {
-        $rs = $this->get_assessments_recordset($id);
-        $assessments = array();
-        foreach ($rs as $assessment) {
-            $assessments[$assessment->id] = $assessment;
-        }
-        $rs->close();
-        return $assessment;
+    public function get_assessments_by_reviewer($userid) {
+        global $DB;
+
+        $sql = 'SELECT a.*,
+                       reviewer.id AS reviewerid,reviewer.firstname AS reviewerfirstname,reviewer.lastname as reviewerlastname,
+                       s.title,
+                       author.id AS authorid, author.firstname AS authorfirstname,author.lastname as authorlastname
+                  FROM {workshop_assessments} a
+            INNER JOIN {user} reviewer ON (a.userid = reviewer.id)
+            INNER JOIN {workshop_submissions} s ON (a.submissionid = s.id)
+            INNER JOIN {user} author ON (s.userid = author.id)
+                 WHERE s.example = 0 AND reviewer.id = :userid AND s.workshopid = :workshopid';
+        $params = array('userid' => $userid, 'workshopid' => $this->id);
+
+        return $DB->get_records_sql($sql, $params);
     }
 
     /**
@@ -677,6 +618,30 @@ class workshop {
         return true;
     }
 
+    /**
+     * Are the peer-reviews available to the authors?
+     *
+     * TODO: this depends on the workshop phase
+     *
+     * @return bool
+     */
+    public function assessments_available() {
+        return true;
+    }
+
+    /**
+     * Can the given grades be displayed to the authors?
+     *
+     * Grades are not displayed if {@link self::assessments_available()} return false. The returned
+     * value may be true (if yes, display grades), false (no, hide grades yet) or null (only
+     * display grades if the assessment has been agreed by the author).
+     *
+     * @return bool|null
+     */
+    public function grades_available() {
+        return true;
+    }
+
     /**
      * Returns the localized name of the grading strategy method to be displayed to the users
      *
@@ -791,7 +756,7 @@ class workshop {
             $rs->close();
             if ($numofsubmissions == 0) {
                 $task->completed = null;
-            } elseif ($numofauthors == $numofallocated) {
+            } elseif ($numofsubmissions == $numofallocated) {
                 $task->completed = true;
             } elseif ($this->phase > self::PHASE_SUBMISSION) {
                 $task->completed = false;
@@ -805,6 +770,13 @@ class workshop {
             $task->details  = get_string('allocatedetails', 'workshop', $a);
             unset($a);
             $phase->tasks['allocate'] = $task;
+
+            if ($numofsubmissions < $numofauthors and $this->phase >= self::PHASE_SUBMISSION) {
+                $task = new stdClass();
+                $task->title = get_string('someuserswosubmission', 'workshop');
+                $task->completed = 'info';
+                $phase->tasks['allocateinfo'] = $task;
+            }
         }
 
         // Prepare tasks for the peer-assessment phase (includes eventual self-assessments)
@@ -812,7 +784,7 @@ class workshop {
         $phase->title = get_string('phaseassessment', 'workshop');
         $phase->tasks = array();
         $phase->isreviewer = has_capability('mod/workshop:peerassess', $this->context, $userid);
-        $phase->assessments = $this->get_assessments($userid); // todo make sure this does not contain assessment of examples
+        $phase->assessments = $this->get_assessments_by_reviewer($userid);
         $numofpeers     = 0;    // number of allocated peer-assessments
         $numofpeerstodo = 0;    // number of peer-assessments to do
         $numofself      = 0;    // number of allocated self-assessments - should be 0 or 1
@@ -833,7 +805,11 @@ class workshop {
         unset($a);
         if ($numofpeers) {
             $task = new stdClass();
-            $task->completed = ($numofpeerstodo == 0);
+            if ($numofpeerstodo == 0) {
+                $task->completed = true;
+            } elseif ($this->phase > self::PHASE_ASSESSMENT) {
+                $task->completed = false;
+            }
             $a = new stdClass();
             $a->total = $numofpeers;
             $a->todo  = $numofpeerstodo;
@@ -844,7 +820,11 @@ class workshop {
         }
         if ($numofself) {
             $task = new stdClass();
-            $task->completed = ($numofselftodo == 0);
+            if ($numofselftodo == 0) {
+                $task->completed = true;
+            } elseif ($this->phase > self::PHASE_ASSESSMENT) {
+                $task->completed = false;
+            }
             $task->title = get_string('taskassessself', 'workshop');
             $phase->tasks['assessself'] = $task;
         }
diff --git a/mod/workshop/submission.php b/mod/workshop/submission.php
index 29515e1b7d..ca37e80ee9 100644
--- a/mod/workshop/submission.php
+++ b/mod/workshop/submission.php
@@ -58,11 +58,13 @@ if ($id) { // submission is specified
 $ownsubmission  = $submission->userid == $USER->id;
 $canviewall     = has_capability('mod/workshop:viewallsubmissions', $PAGE->context);
 $cansubmit      = has_capability('mod/workshop:submit', $PAGE->context);
+$isreviewer     = $DB->record_exists('workshop_assessments', array('submissionid' => $submission->id, 'userid' => $USER->id));
 
-if (!$ownsubmission and !$canviewall) {
-    print_error('nopermissions');
-}
-if ($ownsubmission and !$cansubmit) {
+if ($submission->id and ($ownsubmission or $canviewall or $isreviewer)) {
+    // ok you can go
+} elseif (is_null($submission->id) and $cansubmit) {
+    // ok you can go
+} else {
     print_error('nopermissions');
 }
 
@@ -124,15 +126,21 @@ $currenttab = 'submission';
 include(dirname(__FILE__) . '/tabs.php');
 echo $OUTPUT->heading(format_string($workshop->name), 2);
 
+// if in edit mode, display the form to edit the submission
+
 if ($edit and $ownsubmission) {
     $mform->display();
     echo $OUTPUT->footer();
     die();
 }
 
-if (!empty($submission->id)) {
+// else display the submission
+
+if ($submission->id) {
     $wsoutput = $PAGE->theme->get_renderer('mod_workshop', $PAGE);
     echo $wsoutput->submission_full($submission, true);
+} else {
+    echo $OUTPUT->box(get_string('noyoursubmission', 'workshop'));
 }
 
 if ($ownsubmission and $workshop->submitting_allowed()) {
@@ -143,4 +151,36 @@ if ($ownsubmission and $workshop->submitting_allowed()) {
     echo $OUTPUT->button($editbutton);
 }
 
+// and possibly display the submission's review(s)
+
+$canviewallassessments  = false;
+if (has_capability('mod/workshop:viewallassessments', $PAGE->context)) {
+    $canviewallassessments = true;
+} elseif ($ownsubmission and $workshop->assessments_available()) {
+    $canviewallassessments = true;
+} else {
+    $canviewallassessments = false;
+}
+
+$canviewgrades = false;
+if ($isreviewer) {
+    $canviewgrades = true;  // reviewers can always see the grades they gave even they are not available yet
+} elseif ($ownsubmission or $canviewallassessments) {
+    $canviewgrades = $workshop->grades_available(); // bool|null, see the function phpdoc
+    if (!$canviewgrades and has_capability('mod/workshop:viewgradesbeforeagreement', $PAGE->context)) {
+        $canviewgrades = true;
+    }
+}
+
+if ($isreviewer) {
+    // display own assessment
+    $assessment = 
+    $strategy = $workshop->grading_strategy_instance();
+}
+
+if ($canviewallassessments) {
+    // display all assessments (except the eventual own one - that has been already displayed
+    $strategy = $workshop->grading_strategy_instance();
+}
+
 echo $OUTPUT->footer();
diff --git a/mod/workshop/switchphase.php b/mod/workshop/switchphase.php
index 4c4147ff68..e01a11a901 100644
--- a/mod/workshop/switchphase.php
+++ b/mod/workshop/switchphase.php
@@ -35,7 +35,7 @@ $course     = $DB->get_record('course', array('id' => $cm->course), '*', MUST_EX
 $workshop   = $DB->get_record('workshop', array('id' => $cm->instance), '*', MUST_EXIST);
 $workshop   = new workshop($workshop, $cm, $course);
 
-$PAGE->set_url(new moodle_url($workshop->switchphase_user(), array('cmid' => $cmid, 'phase' => $phase));
+$PAGE->set_url(new moodle_url($workshop->switchphase_url($phase), array('cmid' => $cmid, 'phase' => $phase)));
 
 require_login($course, false, $cm);
 require_capability('mod/workshop:switchphase', $PAGE->context);
diff --git a/mod/workshop/view.php b/mod/workshop/view.php
index b145632deb..66ea6e73bd 100644
--- a/mod/workshop/view.php
+++ b/mod/workshop/view.php
@@ -103,12 +103,11 @@ case workshop::PHASE_SUBMISSION:
         $shownames = has_capability('mod/workshop:viewauthornames', $PAGE->context);
         echo $OUTPUT->box_start('generalbox allsubmissions');
         $counter = 0;
-        $rs = $workshop->get_submissions_recordset('all', false);
-        foreach ($rs as $submission) {
+        $submissions = $workshop->get_submissions('all', false);
+        foreach ($submissions as $submission) {
             $counter++;
             echo $wsoutput->submission_summary($submission, $shownames);
         }
-        $rs->close();
         if ($counter == 0) {
             echo $OUTPUT->container(get_string('nosubmissions', 'workshop'), 'nosubmissions');
         }