From 646cf6479b61a9e8f5f9c6edc9ce18341420497c Mon Sep 17 00:00:00 2001 From: Loic d'Anterroches Date: Thu, 29 Oct 2009 13:25:50 +0100 Subject: [PATCH] Fixed issue 145, code reviews don't show up in the timeline. --- src/IDF/Form/ReviewFileComment.php | 69 ++++++++++++++++++- src/IDF/Review.php | 2 + src/IDF/Review/Comment.php | 53 ++++++++++---- src/IDF/Review/Patch.php | 42 ++++++++++- src/IDF/Timeline.php | 2 +- src/IDF/Views/Project.php | 8 +++ src/IDF/Views/Review.php | 5 ++ src/IDF/templates/idf/review/feedfragment.xml | 18 +++++ .../idf/review/review-updated-email.txt | 8 ++- src/IDF/templates/idf/review/view.html | 60 ++++++++++++++-- 10 files changed, 244 insertions(+), 23 deletions(-) create mode 100644 src/IDF/templates/idf/review/feedfragment.xml diff --git a/src/IDF/Form/ReviewFileComment.php b/src/IDF/Form/ReviewFileComment.php index 3d1b22a..fed70bf 100644 --- a/src/IDF/Form/ReviewFileComment.php +++ b/src/IDF/Form/ReviewFileComment.php @@ -30,12 +30,15 @@ class IDF_Form_ReviewFileComment extends Pluf_Form public $files = null; public $patch = null; public $user = null; + public $project = null; public function initFields($extra=array()) { $this->files = $extra['files']; $this->patch = $extra['patch']; $this->user = $extra['user']; + $this->project = $extra['project']; + foreach ($this->files as $filename => $def) { $this->fields[md5($filename)] = new Pluf_Form_Field_Varchar( array('required' => false, @@ -48,6 +51,41 @@ class IDF_Form_ReviewFileComment extends Pluf_Form ), )); } + $this->fields['content'] = new Pluf_Form_Field_Varchar( + array('required' => true, + 'label' => __('General comment'), + 'initial' => '', + 'widget' => 'Pluf_Form_Widget_TextareaInput', + 'widget_attrs' => array( + 'cols' => 58, + 'rows' => 9, + ), + )); + if ($this->user->hasPerm('IDF.project-owner', $this->project) + or $this->user->hasPerm('IDF.project-member', $this->project)) { + $this->show_full = true; + } + if ($this->show_full) { + $this->fields['summary'] = new Pluf_Form_Field_Varchar( + array('required' => true, + 'label' => __('Summary'), + 'initial' => $this->patch->get_review()->summary, + 'widget_attrs' => array( + 'maxlength' => 200, + 'size' => 67, + ), + )); + + $this->fields['status'] = new Pluf_Form_Field_Varchar( + array('required' => true, + 'label' => __('Status'), + 'initial' => $this->patch->get_review()->get_status()->name, + 'widget_attrs' => array( + 'maxlength' => 20, + 'size' => 15, + ), + )); + } } @@ -64,6 +102,16 @@ class IDF_Form_ReviewFileComment extends Pluf_Form throw new Pluf_Form_Invalid(__('You need to provide comments on at least one file.')); } + function clean_content() + { + $content = trim($this->cleaned_data['content']); + if (!$this->show_full and strlen($content) == 0) { + throw new Pluf_Form_Invalid(__('You need to provide your general comment about the proposal.')); + } + return $content; + } + + /** * Save the model in the database. * @@ -80,6 +128,24 @@ class IDF_Form_ReviewFileComment extends Pluf_Form $bc = new IDF_Review_Comment(); $bc->patch = $this->patch; $bc->submitter = $this->user; + $bc->content = $this->cleaned_data['content']; + $review = $this->patch->get_review(); + if ($this->show_full) { + // Compare between the old and the new data + // Status, summary + $changes = array(); + $status = IDF_Tag::add(trim($this->cleaned_data['status']), $this->project, 'Status'); + if ($status->id != $this->patch->get_review()->status) { + $changes['st'] = $status->name; + } + if (trim($this->patch->get_review()->summary) != trim($this->cleaned_data['summary'])) { + $changes['su'] = trim($this->cleaned_data['summary']); + } + // Update the review + $review->summary = trim($this->cleaned_data['summary']); + $review->status = $status; + $bc->changes = $changes; + } $bc->create(); foreach ($this->files as $filename => $def) { if (!empty($this->cleaned_data[md5($filename)])) { @@ -91,8 +157,7 @@ class IDF_Form_ReviewFileComment extends Pluf_Form $c->create(); } } - $this->patch->get_review()->update(); // reindex and put up in - // the list. + $review->update(); // reindex and put up in the list. return $bc; } } diff --git a/src/IDF/Review.php b/src/IDF/Review.php index 4cc9d60..0d3b8b0 100644 --- a/src/IDF/Review.php +++ b/src/IDF/Review.php @@ -171,6 +171,8 @@ class IDF_Review extends Pluf_Model function postSave($create=false) { + // At creation, we index after saving the associated patch. + if (!$create) IDF_Search::index($this); } /** diff --git a/src/IDF/Review/Comment.php b/src/IDF/Review/Comment.php index cbf4e3d..0407cda 100644 --- a/src/IDF/Review/Comment.php +++ b/src/IDF/Review/Comment.php @@ -117,28 +117,53 @@ class IDF_Review_Comment extends Pluf_Model function postSave($create=false) { - if (0 and $create) { - // Check if more than one comment for this patch. We do - // not want to insert the first comment in the timeline as - // the patch itself is inserted. - $sql = new Pluf_SQL('patch=%s', array($this->patch)); - $co = Pluf::factory(__CLASS__)->getList(array('filter'=>$sql->gen())); - if ($co->count() > 1) { - IDF_Timeline::insert($this, $this->get_patch()->get_review()->get_project(), - $this->get_submitter()); - } + if ($create) { + IDF_Timeline::insert($this, + $this->get_patch()->get_review()->get_project(), + $this->get_submitter()); } - IDF_Search::index($this->get_patch()->get_review()); } public function timelineFragment($request) { - return ''; + $review = $this->get_patch()->get_review(); + $url = Pluf_HTTP_URL_urlForView('IDF_Views_Review::view', + array($request->project->shortname, + $review->id)); + $out = ''. + Pluf_esc(Pluf_Template_dateAgo($this->creation_dtime, 'without')). + ''; + $stag = new IDF_Template_ShowUser(); + $user = $stag->start($this->get_submitter(), $request, '', false); + $ic = (in_array($review->status, $request->project->getTagIdsByStatus('closed'))) ? 'issue-c' : 'issue-o'; + $out .= sprintf(__('Review %3$d, %4$s'), $url, $ic, $review->id, Pluf_esc($review->summary)).''; + $out .= "\n".' +
'.sprintf(__('Update of review %d, by %s'), $url, $ic, $review->id, $user).'
'; + return Pluf_Template::markSafe($out); } public function feedFragment($request) { - return ''; + $review = $this->get_patch()->get_review(); + $url = Pluf_HTTP_URL_urlForView('IDF_Views_Review::view', + array($request->project->shortname, + $review->id)); + $title = sprintf(__('%s: Updated review %d - %s'), + Pluf_esc($request->project->name), + $review->id, Pluf_esc($review->summary)); + $url .= '#ic'.$this->id; + $date = Pluf_Date::gmDateToGmString($this->creation_dtime); + $context = new Pluf_Template_Context_Request( + $request, + array('url' => $url, + 'author' => $this->get_submitter(), + 'title' => $title, + 'c' => $this, + 'review' => $review, + 'date' => $date) + ); + $tmpl = new Pluf_Template('idf/review/feedfragment.xml'); + return $tmpl->render($context); } /** @@ -165,11 +190,13 @@ class IDF_Review_Comment extends Pluf_Model $reviewers[] = $review->get_submitter(); } $comments = $patch->getFileComments(array('order' => 'id DESC')); + $gcomments = $patch->get_comments_list(array('order' => 'id DESC')); $context = new Pluf_Template_Context( array( 'review' => $review, 'patch' => $patch, 'comments' => $comments, + 'gcomments' => $gcomments, 'project' => $prj, 'url_base' => Pluf::f('url_base'), ) diff --git a/src/IDF/Review/Patch.php b/src/IDF/Review/Patch.php index 83186fe..5c9f5da 100644 --- a/src/IDF/Review/Patch.php +++ b/src/IDF/Review/Patch.php @@ -116,6 +116,7 @@ class IDF_Review_Patch extends Pluf_Model function preDelete() { + IDF_Timeline::remove($this); } function preSave($create=false) @@ -127,16 +128,53 @@ class IDF_Review_Patch extends Pluf_Model function postSave($create=false) { + if ($create) { + IDF_Timeline::insert($this, + $this->get_review()->get_project(), + $this->get_review()->get_submitter()); + IDF_Search::index($this->get_review()); + } } public function timelineFragment($request) { - return ''; + $review = $this->get_review(); + $url = Pluf_HTTP_URL_urlForView('IDF_Views_Review::view', + array($request->project->shortname, + $review->id)); + $out = ''. + Pluf_esc(Pluf_Template_dateAgo($this->creation_dtime, 'without')). + ''; + $stag = new IDF_Template_ShowUser(); + $user = $stag->start($review->get_submitter(), $request, '', false); + $ic = (in_array($review->status, $request->project->getTagIdsByStatus('closed'))) ? 'issue-c' : 'issue-o'; + $out .= sprintf(__('Review %3$d, %4$s'), $url, $ic, $review->id, Pluf_esc($review->summary)).''; + $out .= "\n".' +
'.sprintf(__('Creation of review %d, by %s'), $url, $ic, $review->id, $user).'
'; + return Pluf_Template::markSafe($out); } public function feedFragment($request) { - return ''; + $review = $this->get_review(); + $url = Pluf_HTTP_URL_urlForView('IDF_Views_Review::view', + array($request->project->shortname, + $review->id)); + $title = sprintf(__('%s: Creation of Review %d - %s'), + Pluf_esc($request->project->name), + $review->id, Pluf_esc($review->summary)); + $date = Pluf_Date::gmDateToGmString($this->creation_dtime); + $context = new Pluf_Template_Context_Request( + $request, + array('url' => $url, + 'author' => $review->get_submitter(), + 'title' => $title, + 'p' => $this, + 'review' => $review, + 'date' => $date) + ); + $tmpl = new Pluf_Template('idf/review/feedfragment.xml'); + return $tmpl->render($context); } public function notify($conf, $create=true) diff --git a/src/IDF/Timeline.php b/src/IDF/Timeline.php index 2d28ba6..61e6380 100644 --- a/src/IDF/Timeline.php +++ b/src/IDF/Timeline.php @@ -128,7 +128,7 @@ class IDF_Timeline extends Pluf_Model $t->model_id = $item->id; $t->model_class = $item->_model; $t->create(); - return true; + return $t; } /** diff --git a/src/IDF/Views/Project.php b/src/IDF/Views/Project.php index 1cc964f..d66e0ea 100644 --- a/src/IDF/Views/Project.php +++ b/src/IDF/Views/Project.php @@ -92,6 +92,10 @@ class IDF_Views_Project $rights[] = '\'IDF_WikiPage\''; $rights[] = '\'IDF_WikiRevision\''; } + if (true === IDF_Precondition::accessReview($request)) { + $rights[] = '\'IDF_Review_Comment\''; + $rights[] = '\'IDF_Review_Patch\''; + } if (count($rights) == 0) { $rights[] = '\'IDF_Dummy\''; } @@ -169,6 +173,10 @@ class IDF_Views_Project $rights[] = '\'IDF_WikiPage\''; $rights[] = '\'IDF_WikiRevision\''; } + if (true === IDF_Precondition::accessReview($request)) { + $rights[] = '\'IDF_Review_Comment\''; + $rights[] = '\'IDF_Review_Patch\''; + } if (count($rights) == 0) { $rights[] = '\'IDF_Dummy\''; } diff --git a/src/IDF/Views/Review.php b/src/IDF/Views/Review.php index 5316f4d..79671b8 100644 --- a/src/IDF/Views/Review.php +++ b/src/IDF/Views/Review.php @@ -150,6 +150,7 @@ class IDF_Views_Review array('files' => $diff->files, 'user' => $request->user, 'patch' => $patch, + 'project' => $prj, )); if ($form->isValid()) { $review_comment = $form->save(); @@ -166,6 +167,7 @@ class IDF_Views_Review $form = new IDF_Form_ReviewFileComment(null, array('files' => $diff->files, 'user' => $request->user, + 'project' => $prj, 'patch' => $patch,)); } $scm = IDF_Scm::get($request->project); @@ -192,6 +194,7 @@ class IDF_Views_Review } $reviewers = Pluf_Model_RemoveDuplicates($reviewers); return Pluf_Shortcuts_RenderToResponse('idf/review/view.html', + array_merge( array( 'page_title' => $title, 'review' => $review, @@ -202,6 +205,8 @@ class IDF_Views_Review 'form' => $form, 'reviewers' => $reviewers, ), + IDF_Views_Issue::autoCompleteArrays($prj) + ), $request); } } diff --git a/src/IDF/templates/idf/review/feedfragment.xml b/src/IDF/templates/idf/review/feedfragment.xml new file mode 100644 index 0000000..642ef32 --- /dev/null +++ b/src/IDF/templates/idf/review/feedfragment.xml @@ -0,0 +1,18 @@ + + {$title} - {$review.get_status} + + {$url} + {$date} + {$author} +
+{if $c} +
{issuetext $c.content, $request}
+{if $c.changes} +{foreach $c.changes as $w => $v} +{if $w == 'su'}{trans 'Summary:'}{/if}{if $w == 'st'}{trans 'Status:'}{/if}{if $w == 'ow'}{trans 'Owner:'}{/if}{if $w == 'lb'}{trans 'Labels:'}{/if} {if $w == 'lb'}{assign $l = implode(', ', $v)}{$l}{else}{$v}{/if}
+{/foreach} +{/if}{else} +
{issuetext $p.description, $request}
+{/if} +
 + diff --git a/src/IDF/templates/idf/review/review-updated-email.txt b/src/IDF/templates/idf/review/review-updated-email.txt index 52e4049..404688a 100644 --- a/src/IDF/templates/idf/review/review-updated-email.txt +++ b/src/IDF/templates/idf/review/review-updated-email.txt @@ -10,7 +10,13 @@ {assign $tags = $review.get_tags_list()}{if $tags.count()}{trans 'Labels:'} {foreach $tags as $tag} {$tag.class|safe}:{$tag.name|safe} {/foreach} -{/if}{trans 'Comments (last first):'} +{/if}{trans 'General comments (last first):'} + +{foreach $gcomments as $c}{assign $who = $c.get_submitter()}# {blocktrans}By {$who|safe}, {$c.creation_dtime|date}:{/blocktrans} +{$c.content|safe} + +{/foreach} +{trans 'Detailed file comments (last first):'} {foreach $comments as $c}{assign $who = $c.get_comment().get_submitter()}# {blocktrans}By {$who|safe}, {$c.creation_dtime|date}, on file: {$c.cfile|safe} diff --git a/src/IDF/templates/idf/review/view.html b/src/IDF/templates/idf/review/view.html index cadc475..ff9bc23 100644 --- a/src/IDF/templates/idf/review/view.html +++ b/src/IDF/templates/idf/review/view.html @@ -78,10 +78,10 @@ to propose more contributions. {$def[0]} -{assign $comments = $def[2]} -{assign $nc = $comments.count()} +{assign $fcomments = $def[2]} +{assign $nc = $fcomments.count()} {assign $i = 1} -{foreach $comments as $c} +{foreach $fcomments as $c}
{assign $who = $c.get_comment().get_submitter()}{aurl 'whourl', 'IDF_Views_User::view', array($who.login)} {aurl 'url', 'IDF_Views_Review::view', array($project.shortname, $review.id)} {assign $id = $c.id} @@ -100,9 +100,60 @@ to propose more contributions. {/if} {/foreach} + +{assign $i = 1} +{assign $nc = $comments.count()} +{if $nc}
+

{trans 'General Comments'}

+{/if} +{foreach $comments as $c}{ashowuser 'submitter', $c.get_submitter(), $request}{assign $who = $c.get_submitter()} +
+{aurl 'url', 'IDF_Views_Review::view', array($project.shortname, $review.id)} +{assign $id = $c.id} +{assign $url = $url~'#ic'~$c.id} +

{blocktrans}Comment {$i} by {$submitter}, {$c.creation_dtime|date}{/blocktrans}

+ + + +{if strlen($c.content) > 0}
{issuetext $c.content, $request}
{/if} + +{if $c.changedReview()} +
+{foreach $c.changes as $w => $v} +{if $w == 'su'}{trans 'Summary:'}{/if}{if $w == 'st'}{trans 'Status:'}{/if} {$v}
+{/foreach} +
+{/if} +
{assign $i = $i + 1}{if $i == $nc+1 and $user.isAnonymous()} + +{/if} +{/foreach} + {if !$user.isAnonymous()} +{if !$nc}
{/if} + + + +{if $isOwner or $isMember} + + + + + + + +{/if} + @@ -110,10 +161,11 @@ to propose more contributions. {/if} {/block} -u + {block javascript} +{if $isOwner or $isMember}{include 'idf/issues/js-autocomplete.html'}{/if} {/block}
{$form.f.content.labelTag}:{if $form.f.content.errors}{$form.f.content.fieldErrors}{/if} +{$form.f.content|unsafe} +
{$form.f.summary.labelTag}:{if $form.f.summary.errors}{$form.f.summary.fieldErrors}{/if} +{$form.f.summary|unsafe} +
{$form.f.status.labelTag}:{if $form.f.status.errors}{$form.f.status.fieldErrors}{/if} +{$form.f.status|unsafe} +
  | {trans 'Cancel'}