Open Bug 743652 Opened 12 years ago Updated 8 years ago

Move all tr_*.cgi code into Extension.pm

Categories

(Testopia :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: aliustek, Assigned: u430950)

Details

Attachments

(35 files, 32 obsolete files)

10.73 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
6.84 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
35.63 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
10.18 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
14.76 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
9.15 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
56.22 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
28.33 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
24.21 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
21.57 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
30.51 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
17.88 KB, patch
Details | Diff | Splinter Review
13.45 KB, patch
Details | Diff | Splinter Review
13.43 KB, patch
Details | Diff | Splinter Review
13.02 KB, patch
Details | Diff | Splinter Review
13.73 KB, patch
Details | Diff | Splinter Review
11.48 KB, patch
Details | Diff | Splinter Review
33.91 KB, patch
Details | Diff | Splinter Review
42.41 KB, patch
Details | Diff | Splinter Review
11.93 KB, patch
Details | Diff | Splinter Review
18.37 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
37.90 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
46.56 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
9.49 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
28.91 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
33.97 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
16.16 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
21.73 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
14.04 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
19.78 KB, patch
u430950
: review?
Details | Diff | Splinter Review
6.50 KB, patch
u430950
: review?
Details | Diff | Splinter Review
7.15 KB, patch
u430950
: review?
Details | Diff | Splinter Review
25.54 KB, patch
u430950
: review?
Details | Diff | Splinter Review
22.76 KB, patch
u430950
: review?
Details | Diff | Splinter Review
15.25 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
Code for all tr_*.cgi files from Bugzilla root could be moved Testopia/Extension.pm and use page.cgi to do all the work. This would allow all Testopia code to be contained and also Testopia and Bugzilla bzr to not conflict
Attached patch Preliminary Containment Patch (obsolete) — Splinter Review
This a preliminary patch to show the work I did and if it is heading in the correct direction. I used grep and sed to get "tr_*.cgi" to be "page.cgi?id=tr*.cgi" and fixed problems I could see.

Problems detected so far;
*- browsing to show plan/run/case throws an error but still works
*- on show_product page, "Test Plans" tab "New run and "New Case" buttons don't work
To not loose the history of tr_*.cgi scripts, you should use bzr rename instead of bzr remove + bzr add. This would also make the patch *much* smaller.
Comment on attachment 614717 [details] [diff] [review]
Preliminary Containment Patch

>=== modified file 'extensions/Testopia/Extension.pm'

>+sub page_before_template {

>+    my $vars = $args->{vars};

You don't use $vars anywhere, so you don't need this line.


>+    #$page =~ s/\.html$/\.cgi/;
>+    #my $importcgi = "extensions/Testopia/$page";
>+    #eval "require $importcgi";

Use this code instead of the huge if elsif elsif chain. There is one error in your regexp, though. The substitution term must be .cgi, not \.cgi.



>=== modified file 'extensions/Testopia/js/util.js'

>+        return '<a href="page.cgi?id=tr_show_' + type + '.cgi&' + type +'_id=' + id + '" target="_blank">' + id + '</a>';
>+    return '<a href="page.cgi?id=tr_show_' + type +'.cgi&' + type + '_id=' + id + '">' + id + '</a>';

For both lines, .cgi& must be .html&.



>=== modified file 'extensions/Testopia/t/SE_cases.t'

>+    $sel->open("page.cgi?id=tr_show_case.html?case_id=" . TEST_CASE_1);

?case_id= must be &case_id=



>=== modified file 'extensions/Testopia/t/SE_plans.t'

>+    $sel->open("page.cgi?id=tr_show_plan.html?plan_id=" . TEST_PLAN_1);

Same here.



>=== modified file 'extensions/Testopia/template/en/default/testopia/admin/plantypes/add.html.tmpl'

>+<form method="POST" action="page.cgi?id=tr_admin.html">

You cannot pass arguments in the action attribute of a <form>. You must pass it as a hidden field value:

  <form method="POST" action="page.cgi">
    <input type="hidden" name="id" value="tr_admin.html">



>=== modified file 'extensions/Testopia/template/en/default/testopia/admin/plantypes/edit.html.tmpl'

>+<form method="POST" action="page.cgi?id=tr_admin.html">

Same here.



>=== modified file 'extensions/Testopia/template/en/default/testopia/attachment/choose.html.tmpl'

>+<form method="get" action="page.cgi?id=tr_attachment.html">

Same here.



>=== modified file 'extensions/Testopia/template/en/default/testopia/case/choose.html.tmpl'

>+<form method="get" action="page.cgi?id=tr_show_case.html">

Same here, etc.
Attached patch Testopia Containment bundle (obsolete) — Splinter Review
Assignee: gregaryh → aliustek
Attachment #614717 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #615172 - Flags: review?(LpSolit)
Attached patch Testopia Containment bundle v1.1 (obsolete) — Splinter Review
Updated page_before_template to use regex to generate cgi path to require.

Had to include both testopia.all.js and testopia.all.ycomp.js in to patch as both files were moved from extensions/Testopia to root of Testopia code
Attachment #615172 - Attachment is obsolete: true
Attachment #615172 - Flags: review?(LpSolit)
Attachment #615191 - Flags: review?(LpSolit)
Comment on attachment 615191 [details] [diff] [review]
Testopia Containment bundle v1.1

I don't know which pseudo restful ajax calls ghendricks was talking about in his last email, so requesting review from him too.
Attachment #615191 - Flags: review?(gregaryh)
FYI, I won't review this patch till ghendricks answers my question in comment 6. Also, as this will very likely break code at unexpected places, we shouldn't commit this code before Testopia 2.5 is released. Once a working Testopia tarball is available for Bugzilla 4.2.1, then we can start doing major refactoring as suggested here.
Attached patch Testopia_Containment bundle v1.2 (obsolete) — Splinter Review
Fixed some context menu links on updating cases through cases list on plans page
Attachment #615191 - Attachment is obsolete: true
Attachment #615191 - Flags: review?(gregaryh)
Attachment #615191 - Flags: review?(LpSolit)
Attachment #631829 - Flags: review?(gregaryh)
Target Milestone: --- → 2.6
I found a bug in my code but need some help to fix it the following code in extensions/Testopia/Extension.pm breaks when in mod_perl mode, I can't really remember the exact error message and I don't have a perl instance running right now.
>sub page_before_template {
>    my ($self, $args) = @_;
>    my $page = $args->{page_id};
>    if ($page =~ m{^tr_*}) {
>        $page =~ s/\.html$/.cgi/;
>        trick_taint($page);
>        require "extensions/Testopia/controller/".$page;
>        exit;
>    }
>}
Running a modified version of the patch (where the tr_* files are moved into template/en/default/pages) provided in comment 8, I made the following findings:

The following error appears when selecting the Test Case tab in the dashboard, selecting the test case tab from inside a test plan, and after selecting a plan when creating a new test run:

ModPerl::Util::exit: (120000) exit was called at /srv/www/htdocs/bugzilla/extensions/Testopia/template/en/default/pages/tr_list_cases.cgi line 322Compilation failed in require at /srv/www/htdocs/bugzilla/extensions/Testopia/Extension.pm line 2059. 

Line 322 in tr_list_cases.cgi corresponds to an exit in the following conditional:
>    if ($cgi->param('ctype') eq 'json'){
>        Bugzilla->error_mode(ERROR_MODE_AJAX);
>        print $cgi->header;
>        $vars->{'json'} = $table->to_ext_json;
>        $template->process($format->{'template'}, $vars)
>            || ThrowTemplateError($template->error());
>>>        exit;
>    }

The following error occurs after selecting a test plan when creating a new test case:
ModPerl::Util::exit: (120000) exit was called at /srv/www/htdocs/bugzilla/extensions/Testopia/template/en/default/pages/tr_quicksearch.cgi line 465Compilation failed in require at /srv/www/htdocs/bugzilla/extensions/Testopia/Extension.pm line 2059. ,

Line 465 refers to the exit in the following conditional:
>    elsif ($action eq 'getcomponents'){
>        my $plan = Bugzilla::Extension::Testopia::TestPlan->new({});
>        my $product_id = $cgi->param('product_id');
>        my $q = $cgi->param('query');
> 
>        detaint_natural($product_id);
>        my $prod = $plan->lookup_product($product_id);
>        unless (Bugzilla->user->can_see_product($prod)){
>            print '{ERROR:"You do not have permission to view this product"}';
>            exit;
>        }
>        my $product = Bugzilla::Extension::Testopia::Product->new($product_id);
> 
>        my @comps;
>        foreach my $c (@{$product->components}) {
>            if (!$cgi->param('query') || $c->name =~ m/$q/i) {
>                push @comps, {
>                    'id' => $c->id,
>                    'name' => $c->name,
>                    'qa_contact' => $c->default_qa_contact ? $c->default_qa_contact->login : '',
>                    'product' => $c->product->name,
>                };
>            }
>        }
>        my $json = new JSON;
>        print "{'components':". $json->encode(\@comps) ."}";
>>>        exit;
>    }

Any clue to why this is occurring?
Flags: needinfo?(LpSolit)
By definition, it's pretty weird to put a CGI script under template/.

I think a better fix is to put the content of each CGI script into a separate module under Testopia/lib/ and point to them from Extension.pm. You cannot just do copy & paste, but this shouldn't be too much work, I hope. So for instance, tr_admin.cgi could become Testopia/lib/Admin.pm.

To make things easier both for the patch author and the reviewer, each CGI script should be converted in a separate patch, so that each script can be reviewed and checked in independently.
Flags: needinfo?(LpSolit)
Currently working through the code consolidation method provided by Frédéric in comment 11.

I believe that, upon completion of this bug. This would be a good point to push the trunk as Testopia 2.6 and create a new head and tag in Testopia git, as the code consolidation and Bugzilla 4.4 and 5 support would be a significant code improvement.
Assignee: aliustek → theycallmefish
Attached patch tr_admin.diff (obsolete) — Splinter Review
First of many patches needed to complete this bug. To avoid confusion, I will write each patch as if the previous submitted patches have been already pushed to trunk (as this also requires modifying some existing template files to point elsewhere).
Attachment #8475411 - Flags: review?(LpSolit)
Component: API → General
Attachment #631829 - Attachment is obsolete: true
Attachment #631829 - Flags: review?(gregaryh)
Attached patch tr_reports.diff (obsolete) — Splinter Review
Consolidated all the tr_*_reports.cgi conversions into one patch.

6 of 38 files consolidated so far.
Attachment #8480227 - Flags: review?(LpSolit)
Comment on attachment 8475411 [details] [diff] [review]
tr_admin.diff

>+++ b/extensions/Testopia/lib/Admin.pm

>+use Bugzilla;

You shouldn't call Bugzilla.pm from another module, due to circular dependencies. page.cgi already calls it for you.


>+use Bugzilla::Extension::Testopia::Util;
>+use Bugzilla::Extension::Testopia::Constants;

Are these modules really used here? If not, these dependencies should be removed.


>+our $plan = Bugzilla::Extension::Testopia::TestPlan->new({});
>+our $template = Bugzilla->template;

Ideally, you should define these variables from the subroutines themselves.


>+my ($vars) = @_;
>+my $dbh = Bugzilla->dbh;
>+my $input = Bugzilla->input_params;

You cannot use |my| outside subroutines with mod_perl. They must be moved inside report(). Also, I don't see where $dbh is used.


>+sub report {

>+	my $cgi = Bugzilla->cgi;
>+	print $cgi->header();

Why is this code here instead of letting page.cgi display the correct template?



>+++ b/extensions/Testopia/template/en/default/testopia/admin/plantypes/add.html.tmpl

>-<form method="POST" action="tr_admin.cgi">
>+<form method="POST" action="page.cgi?id=tr_admin.html">

I don't think it's legal to pass a query string in action="". You should put it in a hidden field instead.



>+++ b/extensions/Testopia/template/en/default/testopia/admin/plantypes/edit.html.tmpl

>-<form method="POST" action="tr_admin.cgi">
>+<form method="POST" action="page.cgi?id=tr_admin.html">

Same here.


Otherwise looks good. r=LpSolit
Attachment #8475411 - Flags: review?(LpSolit) → review+
(In reply to Frédéric Buclin from comment #15) 
> >+sub report {
> 
> >+	my $cgi = Bugzilla->cgi;
> >+	print $cgi->header();
> 
> Why is this code here instead of letting page.cgi display the correct
> template?
When I remove these two lines, I get an error message about a malformed header in page.cgi
 
> >+++ b/extensions/Testopia/template/en/default/testopia/admin/plantypes/add.html.tmpl
> 
> >-<form method="POST" action="tr_admin.cgi">
> >+<form method="POST" action="page.cgi?id=tr_admin.html">
> 
> I don't think it's legal to pass a query string in action="". You should put
> it in a hidden field instead.
> 
> 
> 
> >+++ b/extensions/Testopia/template/en/default/testopia/admin/plantypes/edit.html.tmpl
> 
> >-<form method="POST" action="tr_admin.cgi">
> >+<form method="POST" action="page.cgi?id=tr_admin.html">
> 
> Same here.

I'm not sure if I quite follow what should be done instead, as this appears to work. Could you put me in the right direction?
Flags: needinfo?(LpSolit)
Nevermind, I figured out what you meant by hidden field. Second patch forthcoming.
Flags: needinfo?(LpSolit)
Attached patch tr_admin-v2.diff (obsolete) — Splinter Review
New patch for tr_admin consolidation with fixes requested.
Attachment #8475411 - Attachment is obsolete: true
Attachment #8533807 - Flags: review?(LpSolit)
Attached patch tr_admin-v3.diff (obsolete) — Splinter Review
Forgot to include both staged and unstaged changes.
Attachment #8533807 - Attachment is obsolete: true
Attachment #8533807 - Flags: review?(LpSolit)
Attachment #8533831 - Flags: review?(LpSolit)
This won't be ready on time for 2.6. IMO, such changes worth to bump the version to 3.0.
Target Milestone: 2.6 → ---
Comment on attachment 8533831 [details] [diff] [review]
tr_admin-v3.diff

>+++ b/extensions/Testopia/Extension.pm

>+sub page_before_template {

>+    if ($page eq 'tr_admin.html'){

Nit: add a whitespace between ){.



>+++ b/extensions/Testopia/lib/Admin.pm

>+use Bugzilla::Constants;
>+use Bugzilla::Util;

You must not remove |use Bugzilla::Error|. It exports Throw*Error().


>+#our $plan = Bugzilla::Extension::Testopia::TestPlan->new({});
>+#our $template = Bugzilla->template;
>+
>+#my ($vars) = @_;
>+#my $input = Bugzilla->input_params;

Remove these lines completely.


>+    ThrowUserError("testopia-read-only", { 'object' => {'type' => 'Testopia administration'}}) unless Bugzilla->user->in_group('admin');

This line is too long. Split it before 'unless ...'.


>+    if ($item eq 'plan_type'){
>+        if ($action eq 'edit'){

Nit: add a whitespace: ) {. There are some other places too.


>+            $plan->update_plan_type($type_id, $type_name, $type_desc);
>+            display();

You must pass $plan: display($plan).


>+                $plan->add_plan_type($type_name, $type_desc);
>+                display();

Same here.


>+        else{
>+            display();

And here.


>+sub display {
>+    my ($vars) = @_;
>+    my $plan = Bugzilla::Extension::Testopia::TestPlan->new({});

$plan has already been set in report(). You must write: my $plan = shift;



>+++ b/extensions/Testopia/template/en/default/testopia/admin/plantypes/add.html.tmpl

>+  <input type="hidden" name="id" value= "tr_admin.html" />
>   <input type="hidden" name="item" value="plan_type" />
>   <input type="hidden" name="action" value="doadd" />

For the record, <foo /> is XHTML syntax. In HTML, you can simply write <foo>.


I didn't test this patch yet as I doubt it works if $plan is not passed to display().
Attachment #8533831 - Flags: review?(LpSolit) → review-
Comment on attachment 8480227 [details] [diff] [review]
tr_reports.diff

This patch is huge. Must all the tr_*_reports.cgi be converted in one shot, or is it possible to have one patch per tr_*reports.cgi script?
I will make the fixes listed in comment 21 and post a new patch in the upcoming days.

(In reply to Frédéric Buclin from comment #22)
> Comment on attachment 8480227 [details] [diff] [review]
> tr_reports.diff
> 
> This patch is huge. Must all the tr_*_reports.cgi be converted in one shot,
> or is it possible to have one patch per tr_*reports.cgi script?

I need to make fixes from previous suggestions on tr_admin, so I will see about separating the reports files
Attached patch tr_admin-v4.diffSplinter Review
Patch includes fixes from comment 21.

Note about this patch: I've decided to not include the removal of the original tr_admin.cgi file in this patch. This is to ensure that consolidation efforts and diff files can pertain to one file at a time. This means:

-If a file references another .cgi script that I have not provided a consolidation for, it will still reference that .cgi
-Once the aforementioned .cgi file has been consolidated, the patch for that consolidation will include switching those references.
-Once a patch has been made available, all future consoldations will be done as if the patch has been successfully added to the codebase.

These efforts will allow us to commit changes to the codebase as we go along, while ensuring easy rollback if necessary.
Attachment #8533831 - Attachment is obsolete: true
Attachment #8574739 - Flags: review?(LpSolit)
On the last patch for this bug, I will include the deletion of all unnecessary files as well as the js minify, so all testing that includes .js file diffs should be done with testopia-debug set to 'Developer'. With the .cgi files still in place, this shouldn't break Testopia for anyone running trunk without 'Developer' set.
Comment on attachment 8480227 [details] [diff] [review]
tr_reports.diff

Reports consolidation will be handled later to avoid file conflicts.
Attachment #8480227 - Flags: review?(LpSolit) → review-
Attached patch tr_case_reports.diff (obsolete) — Splinter Review
It appears that I will be able to separate each of the reports files. Here is the consolidation for tr_case_reports.cgi.

Note: I tried to clean up overlap with other non-committed patches as much as possible, but there is some in the Extension.pm file.
Attachment #8480227 - Attachment is obsolete: true
Attachment #8574851 - Flags: review?(LpSolit)
Attached patch tr_case_reports-v2.diff (obsolete) — Splinter Review
New patch for tr_case_reports, removing unnecessary 'use' statements.
Attachment #8574851 - Attachment is obsolete: true
Attachment #8574851 - Flags: review?(LpSolit)
Attachment #8575449 - Flags: review?(LpSolit)
Attached patch tr_caserun_reports.diff (obsolete) — Splinter Review
Patch for tr_caserun_reports.

Note: Patch includes non-type specific changes required for proper function found also in tr_case_reports.
Attachment #8575468 - Flags: review?(LpSolit)
Attached patch tr_plan_reports.diff (obsolete) — Splinter Review
Patch for tr_plan_reports
Attachment #8575531 - Flags: review?(LpSolit)
Patch for tr_product_reports
Attachment #8575571 - Flags: review?(LpSolit)
Patch for tr_run_reports.

Found some slight areas for cleanup for tr_case_reports, tr_plan_reports, and tr_caserun_reports, so new patches forthcoming.
Attachment #8576118 - Flags: review?(LpSolit)
New patch for tr_case_reports
Attachment #8575449 - Attachment is obsolete: true
Attachment #8575449 - Flags: review?(LpSolit)
Attachment #8576124 - Flags: review?(LpSolit)
New patch for tr_plan_reports
Attachment #8575531 - Attachment is obsolete: true
Attachment #8575531 - Flags: review?(LpSolit)
Attachment #8576126 - Flags: review?(LpSolit)
New patch for tr_caserun_reports
Attachment #8575468 - Attachment is obsolete: true
Attachment #8575468 - Flags: review?(LpSolit)
Attachment #8576128 - Flags: review?(LpSolit)
Consolidation for tr_list_cases

Note: There are some areas of overlap with the report diffs where necessary.
Attachment #8576266 - Flags: review?(LpSolit)
Consolidation for tr_list_plans
Attachment #8576903 - Flags: review?(LpSolit)
Consolidation for tr_show_plan

Note: Listed change in js/util.js changes all tr_show_* files to the needed format. I would recommend waiting until all tr_show_* files have been consolidated before including this fix.
Attachment #8576947 - Flags: review?(LpSolit)
Consolidation for tr_show_case
Attachment #8577324 - Flags: review?(LpSolit)
Attached patch tr_show_run.diffSplinter Review
Consolidation for tr_show_run
Attachment #8577381 - Flags: review?(LpSolit)
Attached patch tr_show_product.diff (obsolete) — Splinter Review
Consolidation for tr_show_product

Note: To save myself time, I've had the diff remove NEW files that weren't affected by this particular patch's consolidation efforts. This was a much easier process than manually cutting out modifications to existing files that had consolidations from multiple patches. If the previous method is desired (or if you have an easier way to handle the 38 patches), let me know.
Attachment #8577446 - Flags: review?(LpSolit)
Attached patch tr_list_runs.diff (obsolete) — Splinter Review
Consolidation for tr_list_runs
Attachment #8577473 - Flags: review?(LpSolit)
Comment on attachment 8574739 [details] [diff] [review]
tr_admin-v4.diff

>+++ b/extensions/Testopia/Extension.pm

>+sub page_before_template {

Hooks should be listed alphabetically, i.e. before post_bug_after_creation().



>+++ b/extensions/Testopia/lib/Admin.pm

>+# Source Code Form is subject to the terms of the Mozilla Public

The MPL 2.0 license starts with "# This Source Code Form...".


>+            display($plan);

>+sub display {
>+    my ($vars) = @_;
>+    my $plan = shift;

As you pass $plan, $vars doesn't contain the original $vars but $plan. So this won't work. One way to fix it would be to let the caller set $vars->{plan} = $plan and then pass $vars to display(), but this is a waste of time. IMO, display() should go away and we should call the template from report() directly.

I also removed this big |if ($item eq 'plan_type')| block to save some indentation.

I fixed all these points on checkin to avoid another round. r=LpSolit


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   ca36bc7..0bf2d65  master -> master
Attachment #8574739 - Flags: review?(LpSolit) → review+
To track what must still be done, it's easier to kill .cgi scripts which have been moved into extension/Testopia/. So I removed tr_admin.cgi.

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   20290d1..2f5a32c  master -> master
Target Milestone: --- → 2.6
Comment on attachment 8576124 [details] [diff] [review]
tr_case_reports-v3.diff

>+++ b/extensions/Testopia/lib/Reports/Case.pm

>+use Bugzilla;

Don't call Bugzilla.pm from here.


>+        unless ($case_id) {
>+            my $cgi = Bugzilla->cgi;

$cgi is already defined.


>+        my $case = Bugzilla::Extension::Testopia::TestCase->new($case_id);

You must load this module first.


>+        exit unless $case->canview;

'exit' seems weird. At this point, the page would be blank. No error, no output at all. Is that really what we want? Wouldn't it make more sense to throw an error using ThrowUserError()? Or at least call 'return' to give Bugzilla a chance to exit nicely? I'm doing s/exit/return/ for now, which seems safer.


>+	my $caserun = Bugzilla::Extension::Testopia::TestCaseRun->new({});

Must load this module first too.


>+    else {
>+        $input->{'current_tab', 'case'};
>+        $input->{'viewall', 1};

You cannot store data into $input that way. You must write $input->{foo} = 'bar'. Has this patch been tested? :)


>+            ThrowCodeError("unknown_action", {action => $input->{'report_action'}});

unknown_action is now a user error instead of a code error. ThrowUserError() must be used instead.


>+        my @time = localtime(time());
>+        my $date = sprintf "%04d-%02d-%02d", 1900+$time[5],$time[4]+1,$time[3];
>+        my $filename = "report-" . $date . ".$format->{extension}";

Haha, that's a clone of old Bugzilla code. :) But cleanup can be done later.



>+++ b/extensions/Testopia/template/en/default/testopia/reports/report.html.tmpl

>-        [% formaturl = "$report.report_loc?$report.switchbase&width=$width&height=$height" _ 
>+        [% formaturl = "$report.report_loc&$report.switchbase&width=$width&height=$height" _ 
>                        "&report_action=data" %]
>         <a href="[% formaturl %]&amp;ctype=csv&amp;format=table">Export as CSV</a> 

It is wrong to mix both & and &amp;. &amp; must be used everywhere. I know this mistake was already there. :)


This looks good. I didn't test the code at all, because I think it depends on several other patches as they are closely interacting. So r=LpSolit for now, and we will catch potential regressions in separate bugs.


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   f1c2c3a..99bbb3f  master -> master
Attachment #8576124 - Flags: review?(LpSolit) → review+
Comment on attachment 8576128 [details] [diff] [review]
tr_caserun_reports-v2.diff

>+++ b/extensions/Testopia/lib/Reports/CaseRun.pm

>+        $input->{'current_tab', 'case_run'};
>+        $input->{'viewall', 1};

Must be $input->{'foo'} = 'bar'.


>+            ThrowCodeError("unknown_action", {action => $input->{'report_action'}});

s/Code/User/


Otherwise looks good. r=LpSolit


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   f1e9885..e691a21  master -> master
Attachment #8576128 - Flags: review?(LpSolit) → review+
Comment on attachment 8576126 [details] [diff] [review]
tr_plan_reports-v2.diff

>+++ b/extensions/Testopia/lib/Reports/Plan.pm

>+        my $plan = Bugzilla::Extension::Testopia::TestPlan->new($plan_id);

You must load this module first.


>+        $input->{'current_tab', 'plan'};

Write $input->{foo} = 'bar'.


>+            ThrowCodeError("unknown_action", {action => $input->{'report_action'}});

s/Code/User/


Otherwise looks good. r=LpSolit


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   4fb142b..fb38486  master -> master
Attachment #8576126 - Flags: review?(LpSolit) → review+
Comment on attachment 8575571 [details] [diff] [review]
tr_product_reports.diff

>+++ b/extensions/Testopia/lib/Reports/Product.pm

>+    use Data::Dumper;

This module is unused.


>+        $product = Bugzilla::Extension::Testopia::Product->new($input->{'product_id'});

This module must be loaded first.


>+    if ($action eq 'draw') {

The whole code is in this IF block. To save some indentation, it's better to return now if $action ne 'draw'.


>+        exit unless $product;

s/exit/return/


>+        my $caserun = Bugzilla::Extension::Testopia::TestCaseRun->new({});
>+        my $run = Bugzilla::Extension::Testopia::TestRun->new({});

These modules must be loaded first.


>+        print $cgi->header;

The header has already been printed at this point.


r=LpSolit with these fixes


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   e0196d8..b5fa234  master -> master
Attachment #8575571 - Flags: review?(LpSolit) → review+
Attached patch tr_list_caseruns.diff (obsolete) — Splinter Review
Consolidation for tr_list_caseruns

Note: This patch was made after the patch approvals over the weekend. I tried to clean up any files this consolidation directly affected from previous patches, but there is still the possibility previous fix suggestions may have not been corrected
Attachment #8578122 - Flags: review?(LpSolit)
Attached patch tr_list_environments.diff (obsolete) — Splinter Review
Consolidation for tr_list_environments
Attachment #8578136 - Flags: review?(LpSolit)
Attached patch tr_process_case.diff (obsolete) — Splinter Review
Consolidation for tr_process_case
Attachment #8578194 - Flags: review?(LpSolit)
Attached patch tr_process_plan.diff (obsolete) — Splinter Review
Consolidation for tr_process_plan
Attachment #8578238 - Flags: review?(LpSolit)
Attached patch tr_process_run.diff (obsolete) — Splinter Review
Consolidation for tr_process_run

Note: I've removed from the diff any file not directly modified by this consolidation. If I find time, I will return to the patches started in comment 41 and do the same.
Attachment #8578709 - Flags: review?(LpSolit)
Attached patch tr_builds.diff (obsolete) — Splinter Review
Consolidation for tr_builds
Attachment #8578748 - Flags: review?(LpSolit)
Attached patch tr_categories.diff (obsolete) — Splinter Review
Consolidation for tr_categories
Attachment #8578831 - Flags: review?(LpSolit)
Attached patch tr_caserun.diff (obsolete) — Splinter Review
Consolidation for tr_caserun
Attachment #8578842 - Flags: review?(LpSolit)
Attached patch tr_attachment.diff (obsolete) — Splinter Review
Consolidation for tr_attachment
Attachment #8579406 - Flags: review?(LpSolit)
Attached patch tr_draw.diff (obsolete) — Splinter Review
Consolidation for tr_draw
Attachment #8579416 - Flags: review?(LpSolit)
Attached patch tr_export_environment.diff (obsolete) — Splinter Review
Consolidation for tr_export_environment
Attachment #8579423 - Flags: review?(LpSolit)
Attached patch tr_import_environment.diff (obsolete) — Splinter Review
Consolidation for tr_import_environment

Note: This was an interesting one to consolidate, as the .cgi script didn't work to begin with. My goal with the consolidation was to not introduce any new errors, and I believe I've accomplished that. This whole process will need to be investigated at a later date. The lack of bugs for this function tells me it is probably not used by most users, so we may want to look at dropping this function altogether.
Attachment #8579450 - Flags: review?(LpSolit)
Attached patch tr_importer.diff (obsolete) — Splinter Review
Consolidation of tr_importer
Attachment #8579588 - Flags: review?(LpSolit)
Attached patch tr_new_case.diffSplinter Review
Consolidation for tr_new_case
Attachment #8580113 - Flags: review?(LpSolit)
Consolidation for tr_new_environment
Attachment #8580204 - Flags: review?(LpSolit)
Attached patch tr_new_plan.diffSplinter Review
Consolidation for tr_new_plan
Attachment #8580218 - Flags: review?(LpSolit)
Attached patch tr_new_run.diff (obsolete) — Splinter Review
Consolidation for tr_new_run
Attachment #8580239 - Flags: review?(LpSolit)
Simple patch. Moving .pl files into extensions/Testopia
Attachment #8580254 - Flags: review?(LpSolit)
Found a mistake in the js/run.js file. Corrected in v2 patch.
Attachment #8580239 - Attachment is obsolete: true
Attachment #8580239 - Flags: review?(LpSolit)
Attachment #8580283 - Flags: review?(LpSolit)
Consolidation for tr_plan_access

Submitting this patch to get another set of eyes looking at the current issue, so this should ultimately be denied.

Current issue: Add/Update works, but delete does not.
Attachment #8588627 - Flags: review?(LpSolit)
Attached patch tr_query.diffSplinter Review
Consolidation for tr_query
Attachment #8588659 - Flags: review?(LpSolit)
Consolidation for tr_quicksearch
Attachment #8588728 - Flags: review?(LpSolit)
Attached patch tr_history.diffSplinter Review
Consolidation for tr_history
Attachment #8588770 - Flags: review?(LpSolit)
Comment on attachment 8576118 [details] [diff] [review]
tr_run_reports.diff

>+++ b/extensions/Testopia/lib/Reports/Run.pm

You must |use JSON| and |use Bugzilla::Extension::Testopia::TestPlan|.


>+        my $dbh = Bugzilla->dbh;

Let's put this line at the beginning of report, so that we don't need to duplicate it.


>+    elsif ($type eq 'changed') {
>+        my $query =
>+            "SELECT case_run_id, case_run_status_id, close_date, from test_case_runs t
>+              INNER JOIN test_runs on test_runs.run_id = t.run_id
>+              WHERE test_runs.plan_id = ?
>+              ORDER BY case_id, close_date DESC";
>+    }

This block is useless as $query is localized and never used inside this ELSIF block.


r=LpSolit with these changes.


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   b5fa234..2f7fc19  master -> master
Attachment #8576118 - Flags: review?(LpSolit) → review+
Comment on attachment 8576266 [details] [diff] [review]
tr_list_cases.diff

>+++ b/extensions/Testopia/lib/List/Cases.pm

>+        my @uneditable;
>+        my @runs;
>+        my @bugs;
>+        my @components;

Nit: they can all be combined on a single line.


>+            next unless $case;
>+            next unless ($case->canview);

Both can be combined into a single: next unless ($case && $case->canview).


>+                else {
>+                    if ($product->id == $case->category->product_id) {

Nit: can be combined into a single elsif.


>+        my @ids = split(",", $input->{'ids'});
>+        my @objs;
>+        foreach my $id (split(",", $input->{'ids'})) {

@objs is unused. @ids is too, but it can be used into the foreach loop instead of calling split() again.


>+        $input->{'current_tab', 'case'};
>+        $input->{'distinct', '1'};

Must write: $input->{foo} = 'bar'.


Everything else looks good, r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   2fb68fd..caa4ea4  master -> master
Attachment #8576266 - Flags: review?(LpSolit) → review+
Comment on attachment 8576903 [details] [diff] [review]
tr_list_plans.diff

Your patches become harder and harder to review, because they contain changes from already committed patches, and so I have to fix conflicts manually, which takes time.


>+++ b/extensions/Testopia/lib/List/Plans.pm

>+use Bugzilla::Config;
>+use Bugzilla::Util;

AFAICT, these two modules are not necessary here.


>+    # Prevent DOS attacks from multiple refreshes of large data
>+    $::SIG{TERM} = 'DEFAULT';
>+    $::SIG{PIPE} = 'DEFAULT';

I wonder if we really need them. I left them here for now.


>+            my $plan = Bugzilla::Extension::Testopia::TestPlan->new($p);

This module must be loaded first.


>+        $input->{'current_tab', 'plan'};
>+        $input->{'distinct', '1'};

As usual, write $input->{foo} = 'bar'.


>+            $vars->{'displaycolumns'} = Bugzilla::Extension::Testopia::TestCase::fields;

This module must be loaded first.


Otherwise looks good. r=LpSolit


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   caa4ea4..ad4e28a  master -> master



I found one problem, though:

When I click the "My Plans" link in the page footer, and then click the "Create a link to this list" link, the URL still points to tr_list_plans.cgi:

  https://localhost/bugzilla-testopia/tr_list_plans.cgi?id=tr_list_plans.html&current_tab=plan&report_type=myplans&limit=25.

It correctly passes id=tr_list_plans.html as a parameter, but it should be page.cgi instead of tr_list_plans.cgi. Any idea?
Attachment #8576903 - Flags: review?(LpSolit) → review+
Comment on attachment 8576947 [details] [diff] [review]
tr_show_plan.diff

>+++ b/extensions/Testopia/lib/Show/Plan.pm

>+use Bugzilla::Extension::Testopia::Util;

I appended qw(percentage) to make clear that we import this subroutine from here which is then passed to $vars->{'percentage'}.


r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   ad4e28a..104a44f  master -> master
Attachment #8576947 - Flags: review?(LpSolit) → review+
Comment on attachment 8577324 [details] [diff] [review]
tr_show_case.diff

r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   104a44f..7954d78  master -> master
Attachment #8577324 - Flags: review?(LpSolit) → review+
Comment on attachment 8577381 [details] [diff] [review]
tr_show_run.diff

r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   de63ffc..66381ba  master -> master
Attachment #8577381 - Flags: review?(LpSolit) → review+
Ryan: could you rediff your 6 patches which are larger than 100 KB (comments 41 - 52)? It's way too much work for me to merge.
Attached patch tr_show_product-v2.diff (obsolete) — Splinter Review
New version of the tr_show_product patch made, as requested.
Attachment #8577446 - Attachment is obsolete: true
Attachment #8577446 - Flags: review?(LpSolit)
Attachment #8659360 - Flags: review?(LpSolit)
I will rewrite the patches from this point forward, so please begin reviewing at comment 79. I will clear the review request on the remaining patches.
Attachment #8577473 - Flags: review?(LpSolit)
Attachment #8578122 - Flags: review?(LpSolit)
Attachment #8578136 - Flags: review?(LpSolit)
Attachment #8578194 - Flags: review?(LpSolit)
Attachment #8578238 - Flags: review?(LpSolit)
Attachment #8578709 - Flags: review?(LpSolit)
Attachment #8578748 - Flags: review?(LpSolit)
Attachment #8578831 - Flags: review?(LpSolit)
Attachment #8578842 - Flags: review?(LpSolit)
Attachment #8579406 - Flags: review?(LpSolit)
Attachment #8579416 - Flags: review?(LpSolit)
Attachment #8579423 - Flags: review?(LpSolit)
Attachment #8579450 - Flags: review?(LpSolit)
Attachment #8579588 - Flags: review?(LpSolit)
Attachment #8580113 - Flags: review?(LpSolit)
Attachment #8580204 - Flags: review?(LpSolit)
Attachment #8580218 - Flags: review?(LpSolit)
Attachment #8580254 - Flags: review?(LpSolit)
Attachment #8580283 - Flags: review?(LpSolit)
Attachment #8588627 - Flags: review?(LpSolit)
Attachment #8588659 - Flags: review?(LpSolit)
Attachment #8588728 - Flags: review?(LpSolit)
Attachment #8588770 - Flags: review?(LpSolit)
Attached patch tr_show_product-v3.diff (obsolete) — Splinter Review
v2 was missing the deletion of the tr_show_product.cgi file.
Attachment #8659360 - Attachment is obsolete: true
Attachment #8659360 - Flags: review?(LpSolit)
Attachment #8659370 - Flags: review?(LpSolit)
Comment on attachment 8659370 [details] [diff] [review]
tr_show_product-v3.diff

>+++ b/extensions/Testopia/lib/Show/Product.pm

>+    #################################################################################
>+    # tr_show_product.html                                                          #
>+    # Displays product level information including builds, categories, environments #
>+    # and tags as well as provides product level reports.                           #
>+    #                                                                               #
>+    # INTERFACE:                                                                    #
>+    #    product_id: product to display                                             #
>+    #    action:                                                                    #
>+    #                                                                               #
>+    #################################################################################

Is this big comment really useful?


>+    if (exists $cgi->{'start'} || exists $cgi->{param}->{'start'}){

What is $cgi->{param}->{'start'}?? This doesn't mean anything to me. Also, why using the $cgi object here instead of $input? This is the reason why I'm not r+ or r-'ing yet.


>+            $input->{'page', $input->{'start'} == 0 ? 0 : $input->{'start'}/$input->{'limit'}};

You must write: $input->{page} = .... Also, wouldn't it be easier to simply write:

  $input->{page} = $input->{'start'}/$input->{'limit'}};


>+    my $search = Bugzilla::Extension::Testopia::Search->new($cgi);

At some point, we should stop passing a CGI object as argument. First of all, that module can call $cgi itself. Secondly, it should use $input, not $cgi.


>+    my $table = Bugzilla::Extension::Testopia::Table->new('plan', 'page.cgi?id=tr_list_plans.html', $cgi, undef, $search->query);

Same comment about the CGI object. Also, some refactoring would be to only pass "tr_list_plans" and that module will automatically prepend "page.cgi?id=". This should of course be done after the migration is complete.
Attachment #8659370 - Flags: review?(LpSolit) → review-
Made corrections as requested, though I ran into problems with the last two when switching from $cgi to $input, resulting in a "can't call method param on unblessed reference" error pointing to:
extensions/Testopia/lib/Table.pm line 151
extensions/Testopia/lib/Search.pm line 90

These two files need to be patched before I can switch the values.
Attachment #8659370 - Attachment is obsolete: true
Attachment #8670365 - Flags: review?(LpSolit)
Comment on attachment 8670365 [details] [diff] [review]
tr_show_product-v4.diff

Looks good. r=LpSolit
Attachment #8670365 - Flags: review?(LpSolit) → review+
To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   66381ba..b8d8197  master -> master
Ryan, do you agree that tr_insanity.pl can be killed? It still calls $dbh->bz_lock_tables which is gone since Bugzilla 3.2, and nobody ever reported that this script was broken.
(In reply to Frédéric Buclin from comment #86)
> Ryan, do you agree that tr_insanity.pl can be killed? It still calls
> $dbh->bz_lock_tables which is gone since Bugzilla 3.2, and nobody ever
> reported that this script was broken.

I think this would be a prime candidate for removal. Nothing ever calls it and it hasn't been updated in about 8 years.
OK, killed.

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   b8d8197..a9f6258  master -> master
Attachment #8577473 - Attachment is obsolete: true
Attachment #8671468 - Flags: review?(LpSolit)
Comment on attachment 8671468 [details] [diff] [review]
tr_list_runs-v2.diff

Looks good. r=LpSolit
Attachment #8671468 - Flags: review?(LpSolit) → review+
I moved "print $cgi->header" near the top of the new .pm file, because it was called in each "if-elsif" block. I also split some very long lines.

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   a9f6258..3ee1c06  master -> master
New patch for tr_list_caseruns.

Note: In consolidating this bug, I discovered that the tr_list_caseruns file (even in its previous .cgi form) does not properly display within the confines of its extJS containers. As the purpose of this bug is to replicate current functionality, I kept the issue in (which has been in the Testopia codebase for quite some time) and created bug 1229135 to fix the problem.
Attachment #8578122 - Attachment is obsolete: true
Attachment #8693754 - Flags: review?(LpSolit)
New patch for tr_list_environments
Attachment #8578136 - Attachment is obsolete: true
Attachment #8693795 - Flags: review?(LpSolit)
New patch for tr_process_case
Attachment #8578194 - Attachment is obsolete: true
Attachment #8694244 - Flags: review?(LpSolit)
Comment on attachment 8693754 [details] [diff] [review]
tr_list_caseruns-v2.diff

>+++ b/extensions/Testopia/js/caserun.js
>-                        url: 'tr_list_caseruns.cgi',
>+                        url: 'page.cgi',
>                         params: {
>+                            id: 'tr_list_caseruns.cgi',

id: 'tr_list_caseruns.html', not .cgi.



>+++ b/extensions/Testopia/lib/List/CaseRuns.pm

>+sub report {

Missing Bugzilla->login(LOGIN_REQUIRED).


>+        $status_id = $input->{'status_id'} if $input->{'status_id'};
>+        detaint_natural($status_id);

detaint_natural() is called without any action if it's not a valid integer. This code is already present in the old .cgi script, but this looks wrong to me.


>+            @case_ids = $input->{'ids'};

$input->{'ids'} returs an arrayref, not an array. So it must be enclosed in @{ }.


>+        my $format = $template->get_format("testopia/caserun/list", scalar $input->{'format'}, scalar $input->{'ctype'});

There is no need to call scalar, because $input will either return a scalar or a ref.


>+        my @time = localtime(time());
>+        my $date = sprintf "%04d-%02d-%02d", 1900+$time[5],$time[4]+1,$time[3];
>+        my $filename = "testresults-$date.$format->{extension}";
>+        print $cgi->header( -type => $format->{'ctype'},
>+                            -content_disposition => "$disp; filename=$filename");

At some point, we will really have to move to the new code to avoid all these ugly lines. Something to keep in mind for 3.2. :)


Your patch looks good, so r=LpSolit. I fixed these problems above on checkin.

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   3ee1c06..8703d68  master -> master
Attachment #8693754 - Flags: review?(LpSolit) → review+
New patch for tr_process_plan
Attachment #8578238 - Attachment is obsolete: true
Attachment #8694315 - Flags: review?(LpSolit)
New patch for tr_process_run.
Attachment #8578709 - Attachment is obsolete: true
Attachment #8694418 - Flags: review?(LpSolit)
Comment on attachment 8693795 [details] [diff] [review]
tr_list_environments-v2.diff

Looks good. r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   97b81ca..6c13430  master -> master
Attachment #8693795 - Flags: review?(LpSolit) → review+
Comment on attachment 8694244 [details] [diff] [review]
tr_process_case-v2.diff

>+++ b/extensions/Testopia/lib/Process/Case.pm

>+    unless ($case) {
>+        print $cgi->header;
>+        ThrowUserError("invalid-test-id-non-existent", {'type' => 'case', id => $input->{'case_id'}}) unless $case;

|unless $case| is useless as it's already the condition to enter this block.


>+        my $newtcaction = $input->{'tcaction'} || '' if $input->{'tcaction'};
>+        my $newtceffect = $input->{'tceffect'} || '' if $input->{'tceffect'};
>+        my $newtcsetup  = $input->{'tcsetup'} || '' if $input->{'tcsetup'};
>+        my $newtcbreakdown = $input->{'tcbreakdown'} || '' if $input->{'tcbreakdown'};

This code is evil. If $input->{foo} is false, is |my $newfoo| initialized or not? This is bad practice. All the |if $foo| must go away.


>+        foreach my $field (qw(action effect)) {
>+            $case->{text}->{$field} =~ s/(<br[\s\/>]+|<p.*?>|<li.*?>)/\n/g;
>+            $case->{text}->{$field} =~ s/<.*?>//g;
>+            # Trivial HTML tag remover
>+            $case->{text}->{$field} =~ s/<[^>]*>//g;
>+            # And this basically reverses the html filter.
>+            $case->{text}->{$field} =~ s/\&#64;/@/g;
>+            $case->{text}->{$field} =~ s/\&lt;/</g;
>+            $case->{text}->{$field} =~ s/\&gt;/>/g;
>+            $case->{text}->{$field} =~ s/\&quot;/\"/g;
>+            $case->{text}->{$field} =~ s/\&nbsp;/ /g;
>+            $case->{text}->{$field} =~ s/\&amp;/\&/g;
>+        }

This code has been stolen from |FILTER txt| in Bugzilla::Template. So to stay in sync with this filter, $case should be passed as is to the template, and the template should use [% case.text.action FILTER txt %] instead of [% ... FILTER none %].


r=LpSolit with these fixes on checkin.

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   351579a..6031737  master -> master
Attachment #8694244 - Flags: review?(LpSolit) → review+
Ryan, do we still need .project in the root directory? This file will have to go away sooner or later. Or at least be moved into extensions/Testopia/.
Comment on attachment 8694315 [details] [diff] [review]
tr_process_plan-v2.diff

>+++ b/extensions/Testopia/lib/Process/Plan.pm

>+    Bugzilla->login(LOGIN_REQUIRED);

This call returns a Bugzilla::User object, which you can then use instead of Bugzilla->user.


>+    unless ($plan) {
>+        print $cgi->header;
>+        ThrowUserError("invalid-test-id-non-existent", {'type' => 'plan', id => $input->{'plan_id'}}) unless $plan;

|unless $plan| is useless as that's already the condition to enter this block.


>+        my $product = Bugzilla::Extension::Testopia::Product->new($product_id);
>+        $product ||= $plan->product;
>+
>+        detaint_natural($product_id);

This code looks wrong to me, for 2 reasons:
- If $product_id is not a valid integer, detaint_natural() will nuke it and no code will catch this problem.
- If $product_id doesn't point to a valid product, $product falls back to $plan->product, but we still use $product_id here despite it won't match $product->id.

For these reasons, I'm killing this detaint_natural() line and use $product->id later in the code, which is safe and in sync with $product. Tell me if you object.


>+            ThrowUserError("invalid-test-id-non-existent",
>+                {'id' => $input->{'new_run_build'}, 'type' => 'Build'}) unless $input->{'new_run_build'};
>+            ThrowUserError("invalid-test-id-non-existent",
>+                {'id' => $input->{'new_run_env'}, 'type' => 'Environment'}) unless $input->{'new_run_env'};

I don't see the point to pass $input->{foo} to ThrowUserError() when the reason for this call is because this value is not set (it can be '' or 0 at most). But that's out of the scope of this bug.


>+        my $newplanid = $plan->clone($plan_name, $author, $product_id, $version, $input->{'copy_doc'});

This is the single line which used $product_id. As explained above, I replaced $product_id by $product->id.


Otherwise looks good. r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   c423296..ad70fb0  master -> master
Attachment #8694315 - Flags: review?(LpSolit) → review+
Comment on attachment 8694418 [details] [diff] [review]
tr_process_run-v2.diff

Looks good. r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   ad70fb0..50181bc  master -> master
Attachment #8694418 - Flags: review?(LpSolit) → review+
(In reply to Ryan Wilson [:f1sh] from comment #60)
> Consolidation for tr_import_environment
> 
> [...] the .cgi script didn't work
> [...] it is probably not used by most users, so we may want to look at
> dropping this function altogether.

In that case, please do. :) Much less code for me to review and for you to port.
f1sh: ping? any progress?
Picking up on consolidation again today.
New patch for tr_builds
Attachment #8578748 - Attachment is obsolete: true
Attachment #8759231 - Flags: review?(LpSolit)
Comment on attachment 8759231 [details] [diff] [review]
tr_builds-v2.diff

>+++ b/extensions/Testopia/lib/Builds.pm

>+    print "Location: page.cgi?id=tr_show_product.html&tab=build\n\n" unless $action;

Use $cgi->redirect() instead of "Location:". Otherwise looks good. r=LpSolit

To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   50181bc..214cd03  master -> master
Attachment #8759231 - Flags: review?(LpSolit) → review+
New patch for tr_categories.
Attachment #8578831 - Attachment is obsolete: true
Attachment #8761283 - Flags: review?(LpSolit)
Comment on attachment 8761283 [details] [diff] [review]
tr_categories-v2.diff

>+++ b/extensions/Testopia/lib/Categories.pm

>+    elsif ($action eq 'list') {
>+        my $json = new JSON;

$json is unused. I removed it, including |use JSON|. r=LpSolit


To ssh://gitolite3@git.mozilla.org/bugzilla/extensions/Testopia.git
   214cd03..a3925f2  master -> master
Attachment #8761283 - Flags: review?(LpSolit) → review+
Attached patch tr_caserun-v2.diff (obsolete) — Splinter Review
New patch for tr_caserun.

In testing this, I found an issue with the 'update' action of tr_list_caseruns. Errors like this occur:
> encountered CODE(0x1e22538), but JSON can only represent references to arrays or hashes at local/lib/perl5/JSON.pm line 154.
This matches the to_json command run at the end of the 'update' action, from what I can tell. Not sure why at this point, but I'll need to fix this before the next release.
Attachment #8578842 - Attachment is obsolete: true
Attachment #8764034 - Flags: review?(LpSolit)
New patch for tr_attachment
Attachment #8579406 - Attachment is obsolete: true
Attachment #8764306 - Flags: review?(LpSolit)
Attached patch tr_draw-v2.diffSplinter Review
New patch for tr_draw.
Attachment #8579416 - Attachment is obsolete: true
Attachment #8764320 - Flags: review?(LpSolit)
New patch for tr_export_environment
Attachment #8579423 - Attachment is obsolete: true
Attachment #8764328 - Flags: review?(LpSolit)
Attached patch tr_import_environment-v2.diff (obsolete) — Splinter Review
New patch for tr_import_environment
Attachment #8579450 - Attachment is obsolete: true
Attachment #8764340 - Flags: review?(LpSolit)
Small mistake on v2.
Attachment #8764340 - Attachment is obsolete: true
Attachment #8764340 - Flags: review?(LpSolit)
Attachment #8764344 - Flags: review?(LpSolit)
New patch for tr_importer
Attachment #8579588 - Attachment is obsolete: true
Attachment #8764345 - Flags: review?(LpSolit)
Comment on attachment 8764034 [details] [diff] [review]
tr_caserun-v2.diff

>+++ b/extensions/Testopia/lib/CaseRun.pm

>+use Bugzilla::Bug;
>+use Bugzilla::Extension::Testopia::Search;
>+use Bugzilla::Extension::Testopia::Table;
>+use Bugzilla::Extension::Testopia::TestRun;
>+use Bugzilla::Extension::Testopia::TestCase;

Unless I miss something, these 5 dependencies are useless.


>+        ThrowUserError("testopia-read-only", {'object' => $caserun}) unless $caserun->canedit;

It's way too easy to crash Testopia with invalid data. There is no check to make sure that $caserun is defined. Just pass an invalid ID and you get:

  Can't call method "canedit" on an undefined value at /loader/0x321e698/Bugzilla/Extension/Testopia/CaseRun.pm line 147.

This is not a regression in your code as the check is already missing in the original code, but Testopia is full of such missing checks. Painful! Once the refactoring is done, checks should be improved everywhere.


>+    elsif ($action eq 'update_sortkey') {
>+        ThrowUserError("testopia-read-only", 
>+            {'object' => $caserun}) unless $caserun->canedit;
>+
>+        my $sortkey = $input->{'sortkey'};
>+        ThrowUserError("number_not_numeric", 
>+            {'num' => $sortkey, field => 'index', field_descs =>{'index' => 'index'}}) unless $caserun->canedit;

The 2nd check doesn't make sense as the first check will already throw an error if $caserun->canedit is false. Based on the error message, probably was $sortkey supposed to be sanitized using detaint_natural() and an error thrown if it's not an integer, but this check is already done in set_sortkey() later. So this check should go away.


>+    elsif ($action eq 'getbugs') {
>+        my $bugs;
>+        foreach my $bug (@{$caserun->bugs}) {
>+            $bugs->{'summary'} = $bug->short_desc;
>+            $bugs->{'bug_id'} = $bug->bug_id;
>+        }

This code doesn't make sense. Each loop will override the previous data. This mistake is already present in the original code. I guess you meant to push each new entry into an arrayref. I will fix that on checkin.


>+        Bugzilla->template->process("testopia/case/text.xml.tmpl", $vars) ||
>+            ThrowTemplateError(Bugzilla->template->error());

Simply write $template instead of Bugzilla->template.


Otherwise looks good. r=LpSolit


I cannot commit your patch to git.mozilla.org. This git repo became read-only a few days ago, and so I'm no longer able to commit patches.
Attachment #8764034 - Flags: review?(LpSolit) → review+
Here is the patch with fixes on checkin, including all my review comments. f1sh, please commit it yourself.
Attachment #8764034 - Attachment is obsolete: true
Attachment #8765029 - Flags: review+
Attachment #8764306 - Flags: review?(LpSolit) → review?
Attachment #8764320 - Flags: review?(LpSolit) → review?
Attachment #8764328 - Flags: review?(LpSolit) → review?
Attachment #8764344 - Flags: review?(LpSolit) → review?
Attachment #8764345 - Flags: review?(LpSolit) → review?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: