1062775 - Create a form to create/update bounty tracking tracking attachments

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Custom Bug Entry Forms, defect, P1)

Description

[Raymond Forbes[:rforbes]]

We need a form to create an attachment with a specific description or update the description if the attachment exists.  

The form should have the following fields:

Reporter's Email
Amount Paid
Reported Date
Fixed Date
Awarded Date
Publish
Credit
Credit
Credit

This should output as an attachment with a filename of bugbounty.data and a description using the above fields.  This should be comma delimited and if the field was left blank it should be blank in the description.

All fields should be strings except for: dates, not sure if bugzilla has a date field, if so these should be date fields, otherwise they should be strings.  Publish, this should be a boolean field.

  For example:

[e-mail], , 2014-06-25, , ,false
[e-mail], 1000, 2010-07-16, 2010-08-04, 2011-06-15, true, "Fortiguard Labs",

let me know if there are any questions.

thanks!

Comment 1

[:glob ✱]

the attachment needs to be marked as private, and the contents of the file can just "bounty".

we should add a "Add bounty tracking attachment" link below the attachments table, visible to insert-group users only.

likewise an "Edit bounty" link should be added next to attachments which are private, text/plain, and appear to have a bounty-formatted description (again insider-group visible only).


raymond, are multiple bounty attachments allowed on the same bug?

Comment 2

[Raymond Forbes[:rforbes]]

nope, just one attachment per bug.

thanks!

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to Byron Jones ‹:glob› from comment #1)
> the attachment needs to be marked as private, and the contents of the file
> can just "bounty".

Hopefully self-evident, but only people who can create a private attachment (insider-group) should be able to use this form.

Comment 4

[Raymond Forbes[:rforbes]]

another field to add, right before the "credit" fields add a "Paid" field

is it possible for the form to have a finite set of options?

thanks!

Comment 5

[:glob ✱]

(In reply to Raymond Forbes[:rforbes] from comment #4)
> is it possible for the form to have a finite set of options?

yes - you just need to provide the list of values for each field.

Comment 6

[Raymond Forbes[:rforbes]]

Publish should be Yes or No
Paid should be Paid or Unpaid.

thanks!

Comment 7

[Raymond Forbes[:rforbes]]

hello.  any update on this?

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Hi! After a little more testing, I'll have a patch up for another bmo developer to review.
It may not make this week's push (although that was already delayed) but I can't foresee it not landing by next week.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1062775-v1.patch

For review.

Comment 10

[:glob ✱]

Comment on attachment 8494276 [details] [diff] [review]
bug-1062775-v1.patch

Review of attachment 8494276 [details] [diff] [review]:
-----------------------------------------------------------------

most of this looks good..

looking at the existing bounty attachments, the 'paid' field is missing from most:
eg. [e-mail],0,2014-08-30,2014-09-02,,true,"Anon",http://example.com
the current code drops the first credit field.

::: extensions/BMO/Extension.pm
@@ +203,5 @@
> +    my $bug        = Bugzilla::Bug->check({ id => $input->{bug_id}, cache => 1 });
> +    my $attachment = first { $_ && is_bounty_attachment($_) } @{$bug->attachments};
> +    $vars->{bug}   = $bug;
> +
> +    if ($input->{reporter_email}) {

as the reporter_email is mandatory, it should be validated server-side.
you should add a hidden field to replace its function here.

@@ +205,5 @@
> +    $vars->{bug}   = $bug;
> +
> +    if ($input->{reporter_email}) {
> +        my @fields = qw( reporter_email amount_paid reported_date fixed_date awarded_date publish paid );
> +        my %form =  map { ($_, $input->{$_}) } @fields;

nit: we generally use an arrow .. map { $_ => $input->{$_} }

::: extensions/BMO/template/en/default/pages/attachment_bounty_form.html.tmpl
@@ +7,5 @@
> +  #%]
> +[% IF redirect;
> +    PROCESS  global/redirect.html.tmpl url="show_bug.cgi?id=$bug.id";
> +    RETURN;
> +END %]

as per our irc discussion, page_before_template is called before the cgi headers are returned, so you should use $cgi to redirect:

print Bugzilla->cgi->redirect('show_bug.cgi?id=' . $bug->id);
exit;

@@ +90,5 @@
> +   style = inline_style
> +   javascript = inline_javascript
> +   javascript_urls = [ 'extensions/BMO/web/js/form_validate.js',
> +                       'js/field.js', 'js/util.js' ]
> +   yui = [ "autocomplete", "calendar", "selector" ]

autocomplete isn't used here

@@ +96,5 @@
> +
> +[% USE Bugzilla %]
> +[% cgi = Bugzilla.cgi %]
> +
> +<form id="bounty_form" method="post" action="page.cgi?id=attachment_bounty_form.html"

put "id" in a hidden field instead of part of the action url

Comment 11

[Raymond Forbes[:rforbes]]

yeah, the paid field is a new addition.  my plan was to go back and add it in to current bugs.  will that need to be done first?

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Sorry, I said this in IRC and didn't update the bug -- adding in that exception to the parse_description() function is no bother. 

As for the Credits, I assume there are up to three Credit fields, and they are quoted?
Do the quoted credits ever contain commas?

Comment 13

[Raymond Forbes[:rforbes]]

nope, they will never contain commas.  or, i should say if there were they would be split into their separate fields.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1062775-v2.patch

> looking at the existing bounty attachments, the 'paid' field is missing from
> most:
> eg. [e-mail],0,2014-08-30,2014-09-02,,true,"Anon",http://example.com
> the current code drops the first credit field.

We should handle the older format now, added to the unit test & rewrote with regexp.

> ::: extensions/BMO/Extension.pm
> @@ +203,5 @@
> > +    my $bug        = Bugzilla::Bug->check({ id => $input->{bug_id}, cache => 1 });
> > +    my $attachment = first { $_ && is_bounty_attachment($_) } @{$bug->attachments};
> > +    $vars->{bug}   = $bug;
> > +
> > +    if ($input->{reporter_email}) {

Done -- added custom error message too.

> nit: we generally use an arrow .. map { $_ => $input->{$_} }

I forget why, but I remember occasionally perl parsing map { ... => ... } as a hash rather than a block. Happy that it doesn't seem to do that, fixed.

> as per our irc discussion, page_before_template is called before the cgi
> headers are returned, so you should use $cgi to redirect:
> 
> print Bugzilla->cgi->redirect('show_bug.cgi?id=' . $bug->id);
> exit;

Dylan  re-learns CGI programming, 101.

> @@ +90,5 @@
> > +   style = inline_style
> > +   javascript = inline_javascript
> > +   javascript_urls = [ 'extensions/BMO/web/js/form_validate.js',
> > +                       'js/field.js', 'js/util.js' ]
> > +   yui = [ "autocomplete", "calendar", "selector" ]
> 
> autocomplete isn't used here

Removed. It was used, until I reckoned that the reporter_email is not really a bugzilla account.

> put "id" in a hidden field instead of part of the action url
Done.

Comment 15

[Raymond Forbes[:rforbes]]

going along with comment 11, how hard is it to change the form if we want to change the attachment description?

Comment 16

[Raymond Forbes[:rforbes]]

so, we figured out how to do this without needing the "Paid" field.  Is it possible to remove that one?

Thanks!

Comment 17

[Dylan Hardison [:dylan] (he/him)]

The paid field can be removed. I'll revise the patch for that case.

Comment 18

[Raymond Forbes[:rforbes]]

what is the next step on this?

Comment 19

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1062775-v2.5.patch

This change removes the paid field. review re-assigned to dkl.

Comment 20

[Dylan Hardison [:dylan] (he/him)]

(In reply to Raymond Forbes[:rforbes] from comment #18)
> what is the next step on this?

My apologies, it was intended for this to be assigned for review later last week. The ball should be rolling with the review process now. I am still confident there will be no major issues in the review.

Comment 21

[David Lawrence [:dkl]]

Comment on attachment 8500610 [details] [diff] [review]
bug-1062775-v2.5.patch

Review of attachment 8500610 [details] [diff] [review]:
-----------------------------------------------------------------

Fixes on commit. Have the reporter try it out on bugzilla-dev as well. r=dkl

::: extensions/BMO/Extension.pm
@@ +222,5 @@
> +                                                            isprivate   => 1,
> +                                                            mimetype    => 'text/plain',
> +                                                            data        => 'bounty',
> +                                                            filename    => 'bugbounty.data',
> +                                                            description => format_bounty_attachment_description(\%form) });

nit: keep lines under 80 chars when possible. Maybe rewrite as:

my $attachment = Bugzilla::Attachment->create({
    bug         => $bug,
    isprivate   => 1,	
    mimetype    => 'text/plain',
    data        => 'bounty',
    filename    => 'bugbounty.data',
    description => format_bounty_attachment_description(\%form)
});

Some other places run over as well.

::: extensions/BMO/template/en/default/hook/bug/show-header-end.html.tmpl
@@ +21,5 @@
>                  "document.title = document.title.replace(/^" _ terms.Bug _ " /, '');"
>  %]
> +[% IF user.is_insider %]
> +  [% javascript = javascript _
> +                  "YAHOO.util.Event.onDOMReady(function () { add_bounty_attachment('$bug.bug_id'); });"

('[% bug.bug_id FILTER js %]')

::: extensions/BMO/web/js/attachment_bounty_form.js
@@ +10,5 @@
> +    if (nodes) {
> +        var td = nodes[0];
> +        var a  = document.createElement('a');
> +        a.href = 'page.cgi?id=attachment_bounty_form.html&bug_id=' + bug_id;
> +        a.appendChild(document.createTextNode('Add/Edit bounty tracking attachment'));

I do not see where we can 'Edit' a bounty attachment using this link. Maybe just change to 'Add' since we can edit from the normal 'details' page.

Comment 22

[David Lawrence [:dkl]]

Comment on attachment 8500610 [details] [diff] [review]
bug-1062775-v2.5.patch

Review of attachment 8500610 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/BMO/Extension.pm
@@ +229,5 @@
> +
> +        print Bugzilla->cgi->redirect('show_bug.cgi?id=' . $bug->id);
> +        exit;
> +    }
> +    else {

One other nit. No need for } else { as we exit above.

Comment 23

[Dylan Hardison [:dylan] (he/him)]

There should be an "add bounty" on http://bugzilla-dev.allizom.org for you to test.
It should be available on any bug, but only visible to people in the insider group. The attachment it creates (or modifies) will be marked private.

Comment 24

[Raymond Forbes[:rforbes]]

ok, everything looks good!  is it at all possible to make it so the form can edit the field?

Comment 25

[Dylan Hardison [:dylan] (he/him)]

(In reply to Raymond Forbes[:rforbes] from comment #24)
> ok, everything looks good!  is it at all possible to make it so the form can
> edit the field?

The "Add" link will perform editing. Can you suggest a better wording? "Add/Edit" was rejected during review (and I think in general we dislike slashes-in-verbs).

Comment 26

[Raymond Forbes[:rforbes]]

well look at that! that works great.  ok, we are all good.  when do you think it can be pushed to the live site?

thanks!

Comment 27

[Dylan Hardison [:dylan] (he/him)]

There was going to be a push today, but dkl is not feeling well. The next push could be this week, but definitely no later than next week.

Comment 28

[Raymond Forbes[:rforbes]]

if we could have it by monday next week that would be great.

Comment 29

[Dylan Hardison [:dylan] (he/him)]

Noted!

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   6d2defe..1030251  master -> master