colchange.cgi should use the database to determine buglist-able columns

RESOLVED FIXED in Bugzilla 4.0

Status

()

enhancement
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

3.5.3
Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
6.99 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
Assignee

Description

9 years ago
Now that fielddefs has a "buglist" column, colchange should be getting its information from the database instead of having a static list.
Assignee

Comment 1

9 years ago
Posted patch v1 (obsolete) — Splinter Review
This turned out to be very easy. :-)
Assignee: query-and-buglist → mkanat
Status: NEW → ASSIGNED
Attachment #427023 - Flags: review?(LpSolit)

Comment 2

9 years ago
Comment on attachment 427023 [details] [diff] [review]
v1

>=== modified file 'colchange.cgi'

>+use constant COLUMN_PARAMS => {

'usestatuswhiteboard' is missing.


>+foreach my $param (keys %{ COLUMN_PARAMS() }) {
>+    foreach my $column (@{ COLUMN_PARAMS->{$param} }) {
>+        delete $columns->{$column} if !Bugzilla->params->{$param};
>+    }
>+}

This would be cleaner and more logical as:

  foreach my $param (keys %{ COLUMN_PARAMS() }) {
      next if Bugzilla->params->{$param};
      foreach my $column (@{ COLUMN_PARAMS->{$param} }) {
          delete $columns->{$column};
      }
  }

This way, you check Bugzilla->params->{$param} only once per parameter.


>+foreach my $class (keys %{ COLUMN_CLASSES() }) {
>+    eval("use $class");

This eval() is useless.


Also, the column list in colchange.cgi is now random and pretty confusing. For now, I think it should just be sorted alphabetically.


Otherwise looks good and works fine.
Attachment #427023 - Flags: review?(LpSolit) → review-
Assignee

Comment 3

9 years ago
Posted patch v2Splinter Review
Thanks for catching all that stuff.

  I kept the eval(use) in there, because I don't know if the other modules will keep "use"ing Bugzilla::Keyword and Bugzilla::Flag, so we should explicitly use them here just in case.

  Also, I realized that "relevance" needed to be excluded from the column list, so I did.

  I'm sorting the available columns case-sensitively and alphabetically now.

  I changed the name of the "Full Summary" column so that it sorts together with the short summary column.

  I also changed the names of the realname columns, to re-use the field_descs values properly and also to put a space in "Real Name".
Attachment #427023 - Attachment is obsolete: true
Attachment #432778 - Flags: review?(LpSolit)

Comment 4

9 years ago
Comment on attachment 432778 [details] [diff] [review]
v2

r=LpSolit
Attachment #432778 - Flags: review?(LpSolit) → review+

Updated

9 years ago
Flags: approval+
Assignee

Comment 5

9 years ago
  I forgot to mark this resolved when I did the checkin.

revision 7070
modified:
  Bugzilla/Hook.pm
  colchange.cgi
  template/en/default/list/change-columns.html.tmpl
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 622437

Updated

4 years ago
Blocks: 272480
You need to log in before you can comment on or make changes to this bug.