743652 - Move all tr_*.cgi code into Extension.pm

Status: ASSIGNED

(Testopia :: General, enhancement)

Description

[rojanu]

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

Comment 1

[rojanu]

Attachment: Preliminary Containment Patch

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

Comment 2

[Frédéric Buclin]

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 3

[Frédéric Buclin]

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.

Comment 4

[rojanu]

Attachment: Testopia Containment bundle

Comment 5

[rojanu]

Attachment: Testopia Containment bundle v1.1

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

Comment 6

[Frédéric Buclin]

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.

Comment 7

[Frédéric Buclin]

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.

Comment 8

[rojanu]

Attachment: Testopia_Containment bundle v1.2

Fixed some context menu links on updating cases through cases list on plans page

Comment 9

[rojanu]

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;
>    }
>}

Comment 10

[u430950]

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?

Comment 11

[Frédéric Buclin]

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.

Comment 12

[u430950]

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.

Comment 13

[u430950]

Attachment: tr_admin.diff

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).

Comment 14

[u430950]

Attachment: tr_reports.diff

Consolidated all the tr_*_reports.cgi conversions into one patch.

6 of 38 files consolidated so far.

Comment 15

[Frédéric Buclin]

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

Comment 16

[u430950]

(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?

Comment 17

[u430950]

Nevermind, I figured out what you meant by hidden field. Second patch forthcoming.

Comment 18

[u430950]

Attachment: tr_admin-v2.diff

New patch for tr_admin consolidation with fixes requested.

Comment 19

[u430950]

Attachment: tr_admin-v3.diff

Forgot to include both staged and unstaged changes.

Comment 20

[Frédéric Buclin]

This won't be ready on time for 2.6. IMO, such changes worth to bump the version to 3.0.

Comment 21

[Frédéric Buclin]

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().

Comment 22

[Frédéric Buclin]

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?

Comment 23

[u430950]

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

Comment 24

[u430950]

Attachment: tr_admin-v4.diff

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.

Comment 25

[u430950]

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 26

[u430950]

Comment on attachment 8480227 [details] [diff] [review]
tr_reports.diff

Reports consolidation will be handled later to avoid file conflicts.

Comment 27

[u430950]

Attachment: tr_case_reports.diff

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.

Comment 28

[u430950]

Attachment: tr_case_reports-v2.diff

New patch for tr_case_reports, removing unnecessary 'use' statements.

Comment 29

[u430950]

Attachment: tr_caserun_reports.diff

Patch for tr_caserun_reports.

Note: Patch includes non-type specific changes required for proper function found also in tr_case_reports.

Comment 30

[u430950]

Attachment: tr_plan_reports.diff

Patch for tr_plan_reports

Comment 31

[u430950]

Attachment: tr_product_reports.diff

Patch for tr_product_reports

Comment 32

[u430950]

Attachment: tr_run_reports.diff

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.

Comment 33

[u430950]

Attachment: tr_case_reports-v3.diff

New patch for tr_case_reports

Comment 34

[u430950]

Attachment: tr_plan_reports-v2.diff

New patch for tr_plan_reports

Comment 35

[u430950]

Attachment: tr_caserun_reports-v2.diff

New patch for tr_caserun_reports

Comment 36

[u430950]

Attachment: tr_list_cases.diff

Consolidation for tr_list_cases

Note: There are some areas of overlap with the report diffs where necessary.

Comment 37

[u430950]

Attachment: tr_list_plans.diff

Consolidation for tr_list_plans

Comment 38

[u430950]

Attachment: tr_show_plan.diff

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.

Comment 39

[u430950]

Attachment: tr_show_case.diff

Consolidation for tr_show_case

Comment 40

[u430950]

Attachment: tr_show_run.diff

Consolidation for tr_show_run

Comment 41

[u430950]

Attachment: tr_show_product.diff

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.

Comment 42

[u430950]

Attachment: tr_list_runs.diff

Consolidation for tr_list_runs

Comment 43

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   ca36bc7..0bf2d65  master -> master

Comment 44

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   20290d1..2f5a32c  master -> master

Comment 45

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   f1c2c3a..99bbb3f  master -> master

Comment 46

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   f1e9885..e691a21  master -> master

Comment 47

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   4fb142b..fb38486  master -> master

Comment 48

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   e0196d8..b5fa234  master -> master

Comment 49

[u430950]

Attachment: tr_list_caseruns.diff

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

Comment 50

[u430950]

Attachment: tr_list_environments.diff

Consolidation for tr_list_environments

Comment 51

[u430950]

Attachment: tr_process_case.diff

Consolidation for tr_process_case

Comment 52

[u430950]

Attachment: tr_process_plan.diff

Consolidation for tr_process_plan

Comment 53

[u430950]

Attachment: tr_process_run.diff

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.

Comment 54

[u430950]

Attachment: tr_builds.diff

Consolidation for tr_builds

Comment 55

[u430950]

Attachment: tr_categories.diff

Consolidation for tr_categories

Comment 56

[u430950]

Attachment: tr_caserun.diff

Consolidation for tr_caserun

Comment 57

[u430950]

Attachment: tr_attachment.diff

Consolidation for tr_attachment

Comment 58

[u430950]

Attachment: tr_draw.diff

Consolidation for tr_draw

Comment 59

[u430950]

Attachment: tr_export_environment.diff

Consolidation for tr_export_environment

Comment 60

[u430950]

Attachment: tr_import_environment.diff

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.

Comment 61

[u430950]

Attachment: tr_importer.diff

Consolidation of tr_importer

Comment 62

[u430950]

Attachment: tr_new_case.diff

Consolidation for tr_new_case

Comment 63

[u430950]

Attachment: tr_new_environment.diff

Consolidation for tr_new_environment

Comment 64

[u430950]

Attachment: tr_new_plan.diff

Consolidation for tr_new_plan

Comment 65

[u430950]

Attachment: tr_new_run.diff

Consolidation for tr_new_run

Comment 66

[u430950]

Attachment: testopia_pl_move.diff

Simple patch. Moving .pl files into extensions/Testopia

Comment 67

[u430950]

Attachment: tr_new_run-v2.diff

Found a mistake in the js/run.js file. Corrected in v2 patch.

Comment 68

[u430950]

Attachment: tr_plan_access.diff

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.

Comment 69

[u430950]

Attachment: tr_query.diff

Consolidation for tr_query

Comment 70

[u430950]

Attachment: tr_quicksearch.diff

Consolidation for tr_quicksearch

Comment 71

[u430950]

Attachment: tr_history.diff

Consolidation for tr_history

Comment 72

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   b5fa234..2f7fc19  master -> master

Comment 73

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   2fb68fd..caa4ea4  master -> master

Comment 74

[Frédéric Buclin]

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://[e-mail]/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?

Comment 75

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   ad4e28a..104a44f  master -> master

Comment 76

[Frédéric Buclin]

Comment on attachment 8577324 [details] [diff] [review]
tr_show_case.diff

r=LpSolit

To ssh://[e-mail]/bugzilla/extensions/Testopia.git
   104a44f..7954d78  master -> master

Comment 77

[Frédéric Buclin]

Comment on attachment 8577381 [details] [diff] [review]
tr_show_run.diff

r=LpSolit

To ssh://[e-mail]/bugzilla/extensions/Testopia.git
   de63ffc..66381ba  master -> master

Comment 78

[Frédéric Buclin]

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.

Comment 79

[u430950]

Attachment: tr_show_product-v2.diff

New version of the tr_show_product patch made, as requested.

Comment 80

[u430950]

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.

Comment 81

[u430950]

Attachment: tr_show_product-v3.diff

v2 was missing the deletion of the tr_show_product.cgi file.

Comment 82

[Frédéric Buclin]

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.

Comment 83

[u430950]

Attachment: tr_show_product-v4.diff

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.

Comment 84

[Frédéric Buclin]

Comment on attachment 8670365 [details] [diff] [review]
tr_show_product-v4.diff

Looks good. r=LpSolit

Comment 85

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/extensions/Testopia.git
   66381ba..b8d8197  master -> master

Comment 86

[Frédéric Buclin]

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.

Comment 87

[u430950]

(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.

Comment 88

[Frédéric Buclin]

OK, killed.

To ssh://[e-mail]/bugzilla/extensions/Testopia.git
   b8d8197..a9f6258  master -> master

Comment 89

[u430950]

Attachment: tr_list_runs-v2.diff

Comment 90

[Frédéric Buclin]

Comment on attachment 8671468 [details] [diff] [review]
tr_list_runs-v2.diff

Looks good. r=LpSolit

Comment 91

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   a9f6258..3ee1c06  master -> master

Comment 92

[u430950]

Attachment: tr_list_caseruns-v2.diff

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.

Comment 93

[u430950]

Attachment: tr_list_environments-v2.diff

New patch for tr_list_environments

Comment 94

[u430950]

Attachment: tr_process_case-v2.diff

New patch for tr_process_case

Comment 95

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   3ee1c06..8703d68  master -> master

Comment 96

[u430950]

Attachment: tr_process_plan-v2.diff

New patch for tr_process_plan

Comment 97

[u430950]

Attachment: tr_process_run-v2.diff

New patch for tr_process_run.

Comment 98

[Frédéric Buclin]

Comment on attachment 8693795 [details] [diff] [review]
tr_list_environments-v2.diff

Looks good. r=LpSolit

To ssh://[e-mail]/bugzilla/extensions/Testopia.git
   97b81ca..6c13430  master -> master

Comment 99

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   351579a..6031737  master -> master

Comment 100

[Frédéric Buclin]

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 101

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   c423296..ad70fb0  master -> master

Comment 102

[Frédéric Buclin]

Comment on attachment 8694418 [details] [diff] [review]
tr_process_run-v2.diff

Looks good. r=LpSolit

To ssh://[e-mail]/bugzilla/extensions/Testopia.git
   ad70fb0..50181bc  master -> master

Comment 103

[Frédéric Buclin]

(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.

Comment 104

[Frédéric Buclin]

f1sh: ping? any progress?

Comment 105

[u430950]

Picking up on consolidation again today.

Comment 106

[u430950]

Attachment: tr_builds-v2.diff

New patch for tr_builds

Comment 107

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   50181bc..214cd03  master -> master

Comment 108

[u430950]

Attachment: tr_categories-v2.diff

New patch for tr_categories.

Comment 109

[Frédéric Buclin]

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://[e-mail]/bugzilla/extensions/Testopia.git
   214cd03..a3925f2  master -> master

Comment 110

[u430950]

Attachment: tr_caserun-v2.diff

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.

Comment 111

[u430950]

Attachment: tr_attachment-v2.diff

New patch for tr_attachment

Comment 112

[u430950]

Attachment: tr_draw-v2.diff

New patch for tr_draw.

Comment 113

[u430950]

Attachment: tr_export_environment-v2.diff

New patch for tr_export_environment

Comment 114

[u430950]

Attachment: tr_import_environment-v2.diff

New patch for tr_import_environment

Comment 115

[u430950]

Attachment: tr_import_environment-v3.diff

Small mistake on v2.

Comment 116

[u430950]

Attachment: tr_importer-v2.diff

New patch for tr_importer

Comment 117

[Frédéric Buclin]

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.

Comment 118

[Frédéric Buclin]

Attachment: tr_caserun-v2.1.diff

Here is the patch with fixes on checkin, including all my review comments. f1sh, please commit it yourself.