From 0aa5999bb37698513d66453d98101232942a6af3 Mon Sep 17 00:00:00 2001 From: Thomas Keller Date: Mon, 30 May 2011 14:02:10 +0200 Subject: [PATCH] Finalize ticket relations (closes issue 638) - IssueUpdate.php: use dynamically set field validators for dynamically created fields; let relation_type0 and relation_issue0 exist at any time; check the validity of a user selection and combine the various input fields if possible; do the database updates for links; change the "change" format for labels to a more precise structure and no longer trust on a leading dash for removed labels - IssueCreate.php: change the validator calls and field names - Issue.php (getGroupedRelatedIssues): make it possible to return only a flat list of integers for easier processing - 17AddIssueRelations.php: migrate the previous serialized "changes" format for issue comments to the new, more structured format (up and down) - js-autocomplete.html: add support for multiple input fields - view.html: output relation changes and wrap the related issues stanzas into paragraphs - NEWS.mdtext: note the addition and the need for a specific version of Pluf --- NEWS.mdtext | 8 +- src/IDF/Form/IssueCreate.php | 30 ++-- src/IDF/Form/IssueUpdate.php | 133 +++++++++++++++++- src/IDF/Issue.php | 4 +- src/IDF/IssueComment.php | 12 +- src/IDF/Migrations/17AddIssueRelations.php | 46 ++++++ src/IDF/Views/Issue.php | 2 +- src/IDF/templates/idf/issues/create.html | 10 +- .../templates/idf/issues/js-autocomplete.html | 61 ++++---- src/IDF/templates/idf/issues/view.html | 33 ++++- 10 files changed, 279 insertions(+), 60 deletions(-) diff --git a/NEWS.mdtext b/NEWS.mdtext index 1acf206..7675253 100644 --- a/NEWS.mdtext +++ b/NEWS.mdtext @@ -1,8 +1,14 @@ # InDefero 1.2 - xxx xxx xx xx:xx 2011 UTC +ATTENTION: You need Pluf [324ae60b](http://projects.ceondo.com/p/pluf/source/commit/324ae60b) +or newer to properly run this version of Indefero! + ## New Features -## Bugfixes +- Indefero's issue tracker can now bi-directionally link issues with variable, configurable + terms, such as "is related to", "is blocked by" or "is duplicated by" (issue 638) + +## Bugfixes - monotone zip archive entries now all carry the revision date as mtime (issue 645) - Timeline only displays filter options for items a user has actually access to (issue 655) diff --git a/src/IDF/Form/IssueCreate.php b/src/IDF/Form/IssueCreate.php index 859adb1..c295ca1 100644 --- a/src/IDF/Form/IssueCreate.php +++ b/src/IDF/Form/IssueCreate.php @@ -113,14 +113,14 @@ class IDF_Form_IssueCreate extends Pluf_Form ), )); - $this->fields['relation_type'] = new Pluf_Form_Field_Varchar( + $this->fields['relation_type0'] = new Pluf_Form_Field_Varchar( array('required' => false, 'label' => __('This issue'), 'initial' => current($this->relation_types), 'widget_attrs' => array('size' => 15), )); - $this->fields['relation_issue'] = new Pluf_Form_Field_Varchar( + $this->fields['relation_issue0'] = new Pluf_Form_Field_Varchar( array('required' => false, 'label' => null, 'initial' => '', @@ -253,9 +253,11 @@ class IDF_Form_IssueCreate extends Pluf_Form return $this->cleaned_data['status']; } - function clean_relation_type() + // this method is not called from Pluf_Form directly, but shared for + // among all similar fields + function clean_relation_type($value) { - $relation_type = trim($this->cleaned_data['relation_type']); + $relation_type = trim($value); if (empty($relation_type)) return ''; @@ -272,9 +274,16 @@ class IDF_Form_IssueCreate extends Pluf_Form return $relation_type; } - function clean_relation_issue() + function clean_relation_type0() { - $issues = trim($this->cleaned_data['relation_issue']); + return $this->clean_relation_type($this->cleaned_data['relation_type0']); + } + + // this method is not called from Pluf_Form directly, but shared for + // among all similar fields + function clean_relation_issue($value) + { + $issues = trim($value); if (empty($issues)) return ''; @@ -296,6 +305,11 @@ class IDF_Form_IssueCreate extends Pluf_Form return implode(', ', $issue_ids); } + function clean_relation_issue0() + { + return $this->clean_relation_issue($this->cleaned_data['relation_issue0']); + } + /** * Clean the attachments post failure. */ @@ -360,9 +374,9 @@ class IDF_Form_IssueCreate extends Pluf_Form $issue->setAssoc($tag); } // add relations - $verb = $this->cleaned_data['relation_type']; + $verb = $this->cleaned_data['relation_type0']; $other_verb = $this->relation_types[$verb]; - $related_issues = preg_split('/\s*,\s*/', $this->cleaned_data['relation_issue'], -1, PREG_SPLIT_NO_EMPTY); + $related_issues = preg_split('/\s*,\s*/', $this->cleaned_data['relation_issue0'], -1, PREG_SPLIT_NO_EMPTY); foreach ($related_issues as $related_issue_id) { $related_issue = new IDF_Issue($related_issue_id); $rel = new IDF_IssueRelation(); diff --git a/src/IDF/Form/IssueUpdate.php b/src/IDF/Form/IssueUpdate.php index 4571ab1..c4c7df0 100644 --- a/src/IDF/Form/IssueUpdate.php +++ b/src/IDF/Form/IssueUpdate.php @@ -104,20 +104,51 @@ class IDF_Form_IssueUpdate extends IDF_Form_IssueCreate ), )); - $this->fields['relation_type'] = new Pluf_Form_Field_Varchar( + $idx = 0; + // note: clean_relation_type0 and clean_relation_issue0 already + // exist in the base class + $this->fields['relation_type'.$idx] = new Pluf_Form_Field_Varchar( array('required' => false, 'label' => __('This issue'), 'initial' => current($this->relation_types), 'widget_attrs' => array('size' => 15), )); - $this->fields['relation_issue'] = new Pluf_Form_Field_Varchar( + $this->fields['relation_issue'.$idx] = new Pluf_Form_Field_Varchar( array('required' => false, 'label' => null, 'initial' => '', 'widget_attrs' => array('size' => 10), )); + ++$idx; + $relatedIssues = $this->issue->getGroupedRelatedIssues(array(), true); + foreach ($relatedIssues as $verb => $ids) { + $this->fields['relation_type'.$idx] = new Pluf_Form_Field_Varchar( + array('required' => false, + 'label' => __('This issue'), + 'initial' => $verb, + 'widget_attrs' => array('size' => 15), + )); + $m = 'clean_relation_type'.$idx; + $this->$m = create_function('$form', ' + return $form->clean_relation_type($form->cleaned_data["relation_type'.$idx.'"]); + '); + + $this->fields['relation_issue'.$idx] = new Pluf_Form_Field_Varchar( + array('required' => false, + 'label' => null, + 'initial' => implode(', ', $ids), + 'widget_attrs' => array('size' => 10), + )); + $m = 'clean_relation_issue'.$idx; + $this->$m = create_function('$form', ' + return $form->clean_relation_issue($form->cleaned_data["relation_issue'.$idx.'"]); + '); + + ++$idx; + } + $tags = $this->issue->get_tags_list(); for ($i=1;$i<7;$i++) { $initial = ''; @@ -171,6 +202,48 @@ class IDF_Form_IssueUpdate extends IDF_Form_IssueCreate public function clean() { $this->cleaned_data = parent::clean(); + + // normalize the user's input by removing dublettes and by combining + // ids from identical verbs in different input fields into one array + $normRelatedIssues = array(); + for ($idx = 0; isset($this->cleaned_data['relation_type'.$idx]); ++$idx) { + $verb = $this->cleaned_data['relation_type'.$idx]; + $ids = preg_split('/\s*,\s*/', $this->cleaned_data['relation_issue'.$idx], + -1, PREG_SPLIT_NO_EMPTY); + if (count($ids) == 0) + continue; + + if (!array_key_exists($verb, $normRelatedIssues)) + $normRelatedIssues[$verb] = array(); + foreach ($ids as $id) { + if (!in_array($id, $normRelatedIssues[$verb])) + $normRelatedIssues[$verb][] = $id; + } + } + + // now look at any added / removed ids + $added = $removed = array(); + $relatedIssues = $this->issue->getGroupedRelatedIssues(array(), true); + $added = array_diff_key($normRelatedIssues, $relatedIssues); + $removed = array_diff_key($relatedIssues, $normRelatedIssues); + + $keysToLookAt = array_keys( + array_intersect_key($relatedIssues, $normRelatedIssues) + ); + foreach ($keysToLookAt as $key) { + $a = array_diff($normRelatedIssues[$key], $relatedIssues[$key]); + if (count($a) > 0) + $added[$key] = $a; + $r = array_diff($relatedIssues[$key], $normRelatedIssues[$key]); + if (count($r) > 0) + $removed[$key] = $r; + } + + // cache the added / removed data, so we do not have to + // calculate that again + $this->cleaned_data['_added_issue_relations'] = $added; + $this->cleaned_data['_removed_issue_relations'] = $removed; + // As soon as we know that at least one change was done, we // return the cleaned data and do not go further. if (strlen(trim($this->cleaned_data['content']))) { @@ -230,6 +303,11 @@ class IDF_Form_IssueUpdate extends IDF_Form_IssueCreate return $this->cleaned_data; } } + + if (count($this->cleaned_data['_added_issue_relations']) != 0 || + count($this->cleaned_data['_removed_issue_relations']) != 0) { + return $this->cleaned_data; + } } // no changes! throw new Pluf_Form_Invalid(__('No changes were entered.')); @@ -271,20 +349,22 @@ class IDF_Form_IssueUpdate extends IDF_Form_IssueCreate foreach ($tags as $tag) { if (!Pluf_Model_InArray($tag, $oldtags)) { if (!isset($changes['lb'])) $changes['lb'] = array(); + if (!isset($changes['lb']['add'])) $changes['lb']['add'] = array(); if ($tag->class != 'Other') { - $changes['lb'][] = (string) $tag; //new tag + $changes['lb']['add'][] = (string) $tag; //new tag } else { - $changes['lb'][] = (string) $tag->name; + $changes['lb']['add'][] = (string) $tag->name; } } } foreach ($oldtags as $tag) { if (!Pluf_Model_InArray($tag, $tags)) { if (!isset($changes['lb'])) $changes['lb'] = array(); + if (!isset($changes['lb']['rem'])) $changes['lb']['rem'] = array(); if ($tag->class != 'Other') { - $changes['lb'][] = '-'.(string) $tag; //new tag + $changes['lb']['rem'][] = (string) $tag; //new tag } else { - $changes['lb'][] = '-'.(string) $tag->name; + $changes['lb']['rem'][] = (string) $tag->name; } } } @@ -302,6 +382,47 @@ class IDF_Form_IssueUpdate extends IDF_Form_IssueCreate or ((!is_null($owner) and !is_null($this->issue->get_owner())) and $owner->id != $this->issue->get_owner()->id)) { $changes['ow'] = (is_null($owner)) ? '---' : $owner->login; } + // Issue relations - additions + foreach ($this->cleaned_data['_added_issue_relations'] as $verb => $ids) { + $other_verb = $this->relation_types[$verb]; + foreach ($ids as $id) { + $related_issue = new IDF_Issue($id); + $rel = new IDF_IssueRelation(); + $rel->issue = $this->issue; + $rel->verb = $verb; + $rel->other_issue = $related_issue; + $rel->submitter = $this->user; + $rel->create(); + + $other_rel = new IDF_IssueRelation(); + $other_rel->issue = $related_issue; + $other_rel->verb = $other_verb; + $other_rel->other_issue = $this->issue; + $other_rel->submitter = $this->user; + $other_rel->create(); + } + if (!isset($changes['rel'])) $changes['rel'] = array(); + if (!isset($changes['rel']['add'])) $changes['rel']['add'] = array(); + $changes['rel']['add'][] = $verb.' '.implode(', ', $ids); + } + // Issue relations - removals + foreach ($this->cleaned_data['_removed_issue_relations'] as $verb => $ids) { + foreach ($ids as $id) { + $db = &Pluf::db(); + $table = Pluf::factory('IDF_IssueRelation')->getSqlTable(); + $sql = new Pluf_SQL('verb=%s AND ( + (issue=%s AND other_issue=%s) OR + (other_issue=%s AND issue=%s))', + array($verb, + $this->issue->id, $id, + $this->issue->id, $id)); + $db->execute('DELETE FROM '.$table.' WHERE '.$sql->gen()); + } + + if (!isset($changes['rel'])) $changes['rel'] = array(); + if (!isset($changes['rel']['rem'])) $changes['rel']['rem'] = array(); + $changes['rel']['rem'][] = $verb.' '.implode(', ', $ids); + } // Update the issue $this->issue->batchAssoc('IDF_Tag', $tagids); $this->issue->summary = trim($this->cleaned_data['summary']); diff --git a/src/IDF/Issue.php b/src/IDF/Issue.php index 6346621..8d1ed34 100644 --- a/src/IDF/Issue.php +++ b/src/IDF/Issue.php @@ -169,7 +169,7 @@ class IDF_Issue extends Pluf_Model } } - function getGroupedRelatedIssues($opts = array()) + function getGroupedRelatedIssues($opts = array(), $idsOnly = false) { $rels = $this->get_related_issues_list(array_merge($opts, array( 'view' => 'with_other_issue', @@ -181,7 +181,7 @@ class IDF_Issue extends Pluf_Model if (!array_key_exists($verb, $res)) { $res[$verb] = array(); } - $res[$verb][] = $rel; + $res[$verb][] = $idsOnly ? $rel->other_issue : $rel; } return $res; diff --git a/src/IDF/IssueComment.php b/src/IDF/IssueComment.php index 366b3e2..20c8339 100644 --- a/src/IDF/IssueComment.php +++ b/src/IDF/IssueComment.php @@ -155,10 +155,18 @@ class IDF_IssueComment extends Pluf_Model $out .= __('Owner:'); break; case 'lb': $out .= __('Labels:'); break; + case 'rel': + $out .= __('Relations:'); break; } $out .= ' '; - if ($w == 'lb') { - $out .= Pluf_esc(implode(', ', $v)); + if ($w == 'lb' || $w == 'rel') { + foreach ($v as $t => $ls) { + foreach ($ls as $l) { + if ($t == 'rem') $out .= ''; + $out .= Pluf_esc($l); + if ($t == 'rem') $out .= ' '; + } + } } else { $out .= Pluf_esc($v); } diff --git a/src/IDF/Migrations/17AddIssueRelations.php b/src/IDF/Migrations/17AddIssueRelations.php index 0bf7700..e9f3636 100644 --- a/src/IDF/Migrations/17AddIssueRelations.php +++ b/src/IDF/Migrations/17AddIssueRelations.php @@ -32,6 +32,27 @@ function IDF_Migrations_17AddIssueRelations_up($params=null) $schema = new Pluf_DB_Schema($db); $schema->model = new IDF_IssueRelation(); $schema->createTables(); + + // change the serialization format for added / removed labels in IDF_IssueComment + $comments = Pluf::factory('IDF_IssueComment')->getList(); + foreach ($comments as $comment) { + if (!isset($comment->changes['lb'])) continue; + $changes = $comment->changes; + $adds = $removals = array(); + foreach ($comment->changes['lb'] as $lb) { + if (substr($lb, 0, 1) == '-') + $removals[] = substr($lb, 1); + else + $adds[] = $lb; + } + $changes['lb'] = array(); + if (count($adds) > 0) + $changes['lb']['add'] = $adds; + if (count($removals) > 0) + $changes['lb']['rem'] = $removals; + $comment->changes = $changes; + $comment->update(); + } } function IDF_Migrations_17AddIssueRelations_down($params=null) @@ -40,5 +61,30 @@ function IDF_Migrations_17AddIssueRelations_down($params=null) $schema = new Pluf_DB_Schema($db); $schema->model = new IDF_IssueRelation(); $schema->dropTables(); + + // change the serialization format for added / removed labels in IDF_IssueComment + $comments = Pluf::factory('IDF_IssueComment')->getList(); + foreach ($comments as $comment) { + $changes = $comment->changes; + if (empty($changes)) + continue; + if (isset($changes['lb'])) { + $labels = array(); + foreach ($changes['lb'] as $type => $lbs) { + if (!is_array($lbs)) { + $labels[] = $lbs; + continue; + } + foreach ($lbs as $lb) { + $labels[] = ($type == 'rem' ? '-' : '') . $lb; + } + } + $changes['lb'] = $labels; + } + // while we're at it, remove any 'rel' changes + unset($changes['rel']); + $comment->changes = $changes; + $comment->update(); + } } diff --git a/src/IDF/Views/Issue.php b/src/IDF/Views/Issue.php index 90c8de8..91fdc40 100644 --- a/src/IDF/Views/Issue.php +++ b/src/IDF/Views/Issue.php @@ -404,7 +404,7 @@ class IDF_Views_Issue $issue = Pluf_Shortcuts_GetObjectOr404('IDF_Issue', $match[2]); $prj->inOr404($issue); $comments = $issue->get_comments_list(array('order' => 'id ASC')); - $related_issues = $issue->getGroupedRelatedIssues(array('order' => 'creation_dtime DESC')); + $related_issues = $issue->getGroupedRelatedIssues(array('order' => 'other_issue ASC')); $url = Pluf_HTTP_URL_urlForView('IDF_Views_Issue::view', array($prj->shortname, $issue->id)); diff --git a/src/IDF/templates/idf/issues/create.html b/src/IDF/templates/idf/issues/create.html index e204a54..9d681a7 100644 --- a/src/IDF/templates/idf/issues/create.html +++ b/src/IDF/templates/idf/issues/create.html @@ -63,12 +63,12 @@ -{$form.f.relation_type.labelTag}: +{$form.f.relation_type0.labelTag}: -{if $form.f.relation_type.errors}{$form.f.relation_type.fieldErrors}{/if} -{if $form.f.relation_issue.errors}{$form.f.relation_issue.fieldErrors}{/if} -{$form.f.relation_type|unsafe} -{$form.f.relation_issue|unsafe} +{if $form.f.relation_type0.errors}{$form.f.relation_type0.fieldErrors}{/if} +{if $form.f.relation_issue0.errors}{$form.f.relation_issue0.fieldErrors}{/if} +{$form.f.relation_type0|unsafe} +{$form.f.relation_issue0|unsafe} diff --git a/src/IDF/templates/idf/issues/js-autocomplete.html b/src/IDF/templates/idf/issues/js-autocomplete.html index 8506783..575e1a5 100644 --- a/src/IDF/templates/idf/issues/js-autocomplete.html +++ b/src/IDF/templates/idf/issues/js-autocomplete.html @@ -51,34 +51,39 @@ return row.to; } }); - $("#id_relation_type").autocomplete(auto_relation_types, { - minChars: 0, - width: 310, - matchContains: true, - max: 50, - highlightItem: false, - formatItem: function(row, i, max, term) { - return row.to.replace(new RegExp("(" + term + ")", "gi"), "$1") + " " + row.name + ""; - }, - formatResult: function(row) { - return row.to; - } - }); - $("#id_relation_issue").autocomplete("{/literal}{url 'IDF_Views_Issue::autoCompleteIssueList', array($project.shortname, $issue.id)}{literal}", { - minChars: 0, - width: 310, - matchContains: true, - max: 10, - multiple: true, - delay: 500, - highlightItem: false, - formatItem: function(row, i, max, term) { - return row[1].replace(new RegExp("(" + term + ")", "gi"), "$1") + " " + row[0] + ""; - }, - formatResult: function(row) { - return row[1]; - } - }); + for (var idx = 0; ; ++idx) { + if ($("#id_relation_type" + idx).length == 0) + break; + + $("#id_relation_type" + idx).autocomplete(auto_relation_types, { + minChars: 0, + width: 310, + matchContains: true, + max: 50, + highlightItem: false, + formatItem: function(row, i, max, term) { + return row.to.replace(new RegExp("(" + term + ")", "gi"), "$1") + " " + row.name + ""; + }, + formatResult: function(row) { + return row.to; + } + }); + $("#id_relation_issue" + idx).autocomplete("{/literal}{url 'IDF_Views_Issue::autoCompleteIssueList', array($project.shortname, $issue.id)}{literal}", { + minChars: 0, + width: 310, + matchContains: true, + max: 10, + multiple: true, + delay: 500, + highlightItem: false, + formatItem: function(row, i, max, term) { + return row[1].replace(new RegExp("(" + term + ")", "gi"), "$1") + " " + row[0] + ""; + }, + formatResult: function(row) { + return row[1]; + } + }); + } }); {/literal} //--> diff --git a/src/IDF/templates/idf/issues/view.html b/src/IDF/templates/idf/issues/view.html index 069cd0b..8694725 100644 --- a/src/IDF/templates/idf/issues/view.html +++ b/src/IDF/templates/idf/issues/view.html @@ -40,7 +40,16 @@ {if $i> 0 and $c.changedIssue()}
{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}
+{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 == 'rel'}{trans 'Relations:'}{/if} +{if $w == 'lb' or $w == 'rel'} + {foreach $v as $t => $ls} + {foreach $ls as $l} + {if $t == 'rem'}{/if}{$l}{if $t == 'rem'} {/if} + {/foreach} + {/foreach} +{else} + {$v} +{/if}
{/foreach}
{/if} @@ -120,12 +129,20 @@ -{$form.f.relation_type.labelTag}: +{$form.f.relation_type0.labelTag}: -{if $form.f.relation_type.errors}{$form.f.relation_type.fieldErrors}{/if} -{if $form.f.relation_issue.errors}{$form.f.relation_issue.fieldErrors}{/if} -{$form.f.relation_type|unsafe} -{$form.f.relation_issue|unsafe} +{assign $prevField} +{foreach $form as $field} + {if strpos($field.name, 'relation_type') === 0} + {$field|unsafe} + {assign $prevField = $field} + {/if} + {if strpos($field.name, 'relation_issue') === 0} + {$field|unsafe}
+ {if $prevField.errors}{$prevField.fieldErrors}{/if} + {if $field.errors}{$field.fieldErrors}{/if} + {/if} +{/foreach} @@ -172,15 +189,17 @@

{/if} {if count($related_issues) > 0} {foreach $related_issues as $verb => $rel_issues} +

{blocktrans}This issue {$verb}{/blocktrans}
{foreach $rel_issues as $rel_issue} + class="label" title="{$rel_issue.other_summary}"> {$rel_issue.other_issue} - {$rel_issue.other_summary|shorten:30}
{/foreach} +

{/foreach} {/if}