657564 - Create a "Payment" form

Status: RESOLVED INVALID

(bugzilla.mozilla.org :: General, enhancement)

Description

[:glob ✱]

(from bug 645732 comment 12)

We would love to have a form that can add info to an already existing bug.

The concept will be that the bug created by "Budget Form" will be approved and then you can use another form to add payment info after the event.

The edit "Payment Form" should be formed after this: http://etherpad.mozilla.com:9000/remo-form-payment

(and same visual style as the eps.mentorship form)

the url should be https://bugzilla.mozilla.org/form.reps.payment

Is that possible?

Comment 1

[Dave Miller [:justdave]]

Although it's certainly more secure than email, I'm not sure we want payment data in Bugzilla.  Might want to run this past a few folks first.

Comment 2

[Dave Miller [:justdave]]

I suspect Bugzilla is actually 90% of the way to PCI compliance because of segregated networks and standalone databases and whatnot, but the ease with which someone could accidentally move a bug out of a restricted product puts the permanence of the security in question.  A standalone app to accept and store this date would likely be much safer.

Comment 3

[Dave Miller [:justdave]]

s/date/data/ 

Also of note - we're not encrypting any of this data when it's stored, either.

Comment 4

[Liz Compton [:liz]]

I sent Jim an email asking him about this. I'll let you know what he says.

Comment 5

[Liz Compton [:liz]]

Jim says they should send their reimbursement requests to [e-mail].

Comment 6

[David Lawrence [:dkl]]

So I assume this new payment form is no longer needed then?

dkl

Comment 7

[:glob ✱]

a standalone app makes more sense, and we already have one (bill.com).
marking as wontfix as per comment 5.

Comment 8

[Pierros Papadeas [:pierros]]

I am sorry, but I don't see how we can ask contributors to send their info to bill.com.. Besides we first have to approve those requests and then have someone to actually pay them on these payment info.

How are we going to manage approval, tracking and peer visibility through bill.com?

Comment 9

[Liz Compton [:liz]]

Pierros - Why don't you meet with Bree in accounting, who manages bill.com, explain to her your program, and see what you can figure out.

Comment 10

[Pierros Papadeas [:pierros]]

Thanks Liz, William has already planned a meeting with accounting today, and I will update the bug accordingly.

Comment 11

[Pierros Papadeas [:pierros]]

We just had a meeting with Legal today and Finance yesterday.

I updated the Etherpad to reflect the changes. We are not going to store bank details to bugzilla. Instead they are going to be prompted to email this info to William Q (so that he can process those through bill.com)

Please proceed with the form creation.

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #11)
> We are not going to store
> bank details to bugzilla. Instead they are going to be prompted to email
> this info to William Q (so that he can process those through bill.com)

That's even worse. At least a form post on Bugzilla would have been encrypted in transit to us. Email gets sent in the clear over the net, ripe for anyone on the path to sniff it and get those juicy bank details.

Comment 13

[Dave Miller [:justdave]]

FWIW, bill.com does allow you to upload directly to them via their secure web page.  I think it requires we set up an account for the user first though. :(

Comment 14

[Pierros Papadeas [:pierros]]

:( .. I don't think that we should have an account for each contributor on bill.com.

So the only viable solution is to have the info on bugzilla as initially requested? (is email such a problem?)

Thanks!

Comment 15

[Liz Compton [:liz]]

William and Pierros - You're planning to follow the recommendations of our accounting department, right?

Chris - Can you weigh in from a security perspective?

Comment 16

[Chris Lyon [:clyon]]

We have had problems with forms and "user error" in the past cloning bugs. We also don't encrypt any of this data within the database, as justdave has pointed out. This data just can't be inside of Bugzilla. 

We just shouldn't be taking this data in on Bugzilla. Why can't set examples with these members to use email encryption?

Comment 17

[William Quiviger [:williamq]]

I've reviewed in the detail the whole payment process used for the Mozilla Reps program with the finance team and it was they're recommendation to have Mozilla Reps send me via email their bank details for reimbursement. We're following they're recommendation.

Comment 18

[Pierros Papadeas [:pierros]]

Ping, because this is a blocker for Mozilla Reps program to start, do we have any ETA?

Comment 19

[Chris Lyon [:clyon]]

(In reply to comment #18)
> Ping, because this is a blocker for Mozilla Reps program to start, do we
> have any ETA?

eta for what? a form on bugzilla which will hold bank details?

Comment 20

[Pierros Papadeas [:pierros]]

Did I say that ? :)

Comment 11 and this etherpad has the details: 
http://etherpad.mozilla.com:9000/remo-form-payment

This is what we did on bug 657563 also.

Thanks!

Comment 21

[David Lawrence [:dkl]]

Working on this.

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to create payment form (v1)

Here is a patch that creates a page.cgi form that will append information to a current bug report. The form will prefill a comment and also attach two files to the bug report, expense form and receipts. I will commit to stage-tip as well so feedback can be given.

dkl

Comment 23

[David Lawrence [:dkl]]

pierros, you can give this a try at 

https://bugzilla-stage-tip.mozilla.org/page.cgi?id=remo-form-payment.html

Dkl

Comment 24

[Pierros Papadeas [:pierros]]

Just tested it. Works like a charm :)

Thanks David!

After landing on production please don't forget to create the alias url. (same goes for the other 2 bugs)

Comment 25

[:glob ✱]

Comment on attachment 535485 [details] [diff] [review]
Patch to create payment form (v1)

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

looks good; just a few things to tidy up :)

::: extensions/BMO/Extension.pm
@@ +806,5 @@
> +        my $token  = $input->{'token'};
> +
> +        check_token_data($token, 'remo_form_payment');
> +
> +	    detaint_natural($bug_id);

nit: tabs

@@ +810,5 @@
> +	    detaint_natural($bug_id);
> +        my $bug = Bugzilla::Bug->check($bug_id);
> +
> +        # Make sure the user can attach to this bug
> +        if (!$user->can_edit_product($bug->product_obj->id)) {

this should be $bug->user->{'canedit'}

@@ +886,5 @@
> +        delete_token($token);
> +    
> +        # Define the variables and functions that will be passed to the UI template.
> +        $vars->{'attachment'} = $attachment;
> +        $vars->{'bugs'} = [ new Bugzilla::Bug($bug_id) ];

you should be able to pass in the existing $bug object here.

::: extensions/BMO/template/en/default/pages/comment-remo-form-payment.txt.tmpl
@@ +15,5 @@
> +  # Copyright (C) 1998 Netscape Communications Corporation. All
> +  # Rights Reserved.
> +  #
> +  # Contributor(s): Gervase Markham <[e-mail]>
> +  #%]

the boilerplate needs updating, mozilla foundation, year, name, etc.

::: extensions/BMO/template/en/default/pages/remo-form-payment.html.tmpl
@@ +15,5 @@
> +  # Portions created by Mozilla are Copyright (C) 2008 Mozilla
> +  # Corporation. All Rights Reserved.
> +  #
> +  # Contributor(s): Reed Loden <[e-mail]>
> +  #                 David Tran <[e-mail]>

the boilerplate needs updating, mozilla foundation, year, name, etc.

@@ +24,5 @@
> +[% PROCESS global/header.html.tmpl
> +   title = "Mozilla Reps Payment Form"
> +   style_urls = [ 'extensions/BMO/web/styles/moz_reps.css' ]
> +   javascript_urls = [ 'extensions/BMO/web/js/form_validate.js',
> +		               'js/util.js',

please remove tabs

@@ +117,5 @@
> +</tr>
> +
> +<tr class="even">
> +  <td colspan="2">
> +    <strong>Have you already received payment for this event? <span style="color: red;">*</span></strong>

don't mark this checkbox as mandatory; it's confusing as it implies that it must be checked.

@@ +161,5 @@
> +
> +</form>
> +
> +<p>
> +  <strong><span style="color: red;">*</span></strong> - Required field<br />

nit: we're not xhtml, <br> not <br/>

Comment 26

[:glob ✱]

just thinking some more about this; we should also check that the bug provided is in the correct product and component, to try to catch typos.

bonus points for showing the entered bug's summary when focus leaves the bug id input field.

Comment 27

[David Lawrence [:dkl]]

Thanks

(In reply to comment #25)
> @@ +886,5 @@
> > +        delete_token($token);
> > +    
> > +        # Define the variables and functions that will be passed to the UI template.
> > +        $vars->{'attachment'} = $attachment;
> > +        $vars->{'bugs'} = [ new Bugzilla::Bug($bug_id) ];
> 
> you should be able to pass in the existing $bug object here.

I got the design from process_bug.cgi which states in the comments:

# $bug->update() does not update the internal structure of
# the bug sufficiently to display the bug with the new values.
# (That is, if we just passed in the old Bug object, we'd get
# a lot of old values displayed.)

So I figured it was safest just to reload a fresh bug object.

dkl

Comment 28

[David Lawrence [:dkl]]

Attachment: Patch to create payment form (v2)

Thanks glob. Here is a new patch that addresses your changes as well as adds the bonus points of getting the bug id's status and summary and displaying it beneath the id field.

dkl

Comment 29

[Dave Miller [:justdave]]

I'm not sure which patch to apply this comment to or if it gets a negative review...  the last patch on here doesn't even touch the line in question which makes me think it was already broken before?

This produces an Internal Server Error when you load it.

file error - parse error - hook/global/user-error-errors.html.tmpl line 43: unexpected token ("remo_payment_invalid_product')
  [% ELSIF error == "remo_payment_invalid_product' %]

Comment 30

[Dave Miller [:justdave]]

(In reply to comment #17)
> I've reviewed in the detail the whole payment process used for the Mozilla
> Reps program with the finance team and it was they're recommendation to have
> Mozilla Reps send me via email their bank details for reimbursement. We're
> following they're recommendation.

What I'm saying now is in no way a stop-ship, that's up to you guys and finance to decide. The way it's set up now is the procedure Finance has in place.  I think that procedure is very wrong, but that's my personal opinion, and in no way should it stop this from getting put up for use, since we're doing what they said.

Personally I'd be very unwilling to send you bank account information via email without it being encrypted.  That's just asking for my bank account info to get stolen by someone sniffing packets on the internet.  My personal recommendation would be for you guys provide a GPG public key which can be used to encrypt the emails (and we can put the ascii-armored version of it right in the form so they can copy/paste it to use for encrypting).

Comment 31

[Dave Miller [:justdave]]

Comment on attachment 535721 [details] [diff] [review]
Patch to create payment form (v2)

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

::: extensions/BMO/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +40,4 @@
>      be performed by a [% terms.Bugzilla %] admin. Please file a bug report
>      under 'bugzilla.mozilla.org:Administration' requesting the new field.
>  
> +[% ELSIF error == "remo_payment_invalid_product' %]

Guess it is in this patch...  ^^^  mismatched quotes.

Comment 32

[Dave Miller [:justdave]]

This page produces an ISE if you try to access it without being logged in.

DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`bugstage02/tokens`, CONSTRAINT `fk_tokens_userid_profiles_userid` FOREIGN KEY (`userid`) REFERENCES `profiles` (`userid`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO tokens (userid, issuedate, token, tokentype, eventdata)
        VALUES (?, NOW(), ?, ?, ?)"] at /data/www/bugzilla.mozilla.org/Bugzilla/Token.pm line 425
Bugzilla::Token::_create_token(0, 'session', 'remo_form_payment') called at /data/www/bugzilla.mozilla.org/Bugzilla/Token.pm line 172
Bugzilla::Token::issue_session_token('remo_form_payment') called at /data/www/bugzilla.mozilla.org/extensions/BMO/Extension.pm line 911
Bugzilla::Extension::BMO::_remo_form_payment('HASH(0x90b49ec)') called at /data/www/bugzilla.mozilla.org/extensions/BMO/Extension.pm line 147
Bugzilla::Extension::BMO::page_before_template('Bugzilla::Extension::BMO=HASH(0x8967f6c)', 'HASH(0x896b500)') called at /data/www/bugzilla.mozilla.org/Bugzilla/Hook.pm line 33
Bugzilla::Hook::process('page_before_template', 'HASH(0x896b500)') called at /data/www/bugzilla.mozilla.org/page.cgi line 84

Comment 33

[David Lawrence [:dkl]]

Thanks for finding those justdave. I have fixed both in the BZR repo now.

dkl

Comment 34

[David Lawrence [:dkl]]

Comment on attachment 535721 [details] [diff] [review]
Patch to create payment form (v2)

glob. I fixed the problems justdave found in BZR. Please still review the bits dealing with ajax and other issues you found before. 

Thanks dkl

Comment 35

[:glob ✱]

Comment on attachment 535721 [details] [diff] [review]
Patch to create payment form (v2)

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

if you enter an invalid bug number, you get a "data.result is undefined" javascript error, and the bug id input is disabled.

you don't need to re-request the bug information if the value hasn't changed.

the summary of the test should start with "Bug ". ie.
  Bug 12345 - VERIFIED - ...
instead of
  12345 - VERIFIED - ...
and "Bug 12345" should be a link to the bug in question (open in a new window).

you could test the bug's product and component in javascript, and reject invalid bugs prior to submission.

there's a javascript error when you submit:
  document.getElementById("short_desc") is null

the generated comment needs to say that it's a payment request.

::: .htaccess
@@ +37,4 @@
>  RewriteRule ^form[\.:]reps[\.:]mentorship$ enter_bug.cgi?product=Mozilla+Reps&format=mozreps
>  RewriteRule ^form[\.:]reps[\.:]budget$ enter_bug.cgi?product=Mozilla+Reps&format=remo-budget
>  RewriteRule ^form[\.:]reps[\.:]swag$ enter_bug.cgi?product=Mozilla+Reps&format=remo-swag
> +RewriteRule ^form[\.:]reps[\.:]payment$ page.cgi?id=remo-form-payment

this needs to be page.cgi?id=remo-form-payment.html

Comment 36

[David Lawrence [:dkl]]

Attachment: Patch to create payment form (v3)

Thanks glob. Here is a new patch which addresses all of your changes. I have committed and should be testable on stage-tip.

dkl

Comment 37

[:glob ✱]

Comment on attachment 536232 [details] [diff] [review]
Patch to create payment form (v3)

if you resubmit the form (submit, back, submit again), you get an ISE:

  Undefined subroutine Fh::slice
  at template\en\default\global\hidden-fields.html.tmpl line 60

the crux of this issue is you're using check_token_data(), which is designed to work with the admin pages only -- it uses admin/confirm-action.html.tmpl -- and the name of the upload field is hard-coded as "data".

it's probably easiest at this stage to copy the token validation code from post_bug, however you can't use the same template as it won't post to the correct cgi; just throw a user error stating that they can't submit the same form twice, with a link to the form which will allow them to create a new request.

if we are pressed for time, i'm happy for the current code to go live as long as this is resolved soonish.

Comment 38

[David Lawrence [:dkl]]

Attachment: Patch to create payment form (v4)

Thanks again for the review. I have reworked the token code and submitting a patch with those changes for review.

dkl

Comment 39

[:glob ✱]

Comment on attachment 536315 [details] [diff] [review]
Patch to create payment form (v4)

r=glob :)

Comment 40

[David Lawrence [:dkl]]

This is live now. Please reopen if there are any issues.

https://bugzilla.mozilla.org/form.reps.payment

dkl

Comment 41

[Liz Compton [:liz]]

Sorry for the late request, but I talked to Alex Fowler about this and he recommends that a note be added to the form that says something like "Please black out any bank account information included on receipts before attaching them."

Comment 42

[Liz Compton [:liz]]

William and Pierros - With respect to how reps send William their bank wire info, Alex agreed with Dave that sending by email is too insecure. He suggested that you modify the process just a little by asking them to send you the info either by fax or snail mail (or call you and give it to you over the phone). Hopefully it won't be a problem to make that minor change.

Comment 43

[David Lawrence [:dkl]]

(In reply to comment #41)
> Sorry for the late request, but I talked to Alex Fowler about this and he
> recommends that a note be added to the form that says something like "Please
> black out any bank account information included on receipts before attaching
> them."

Will add that now.

(In reply to comment #42)
> William and Pierros - With respect to how reps send William their bank wire
> info, Alex agreed with Dave that sending by email is too insecure. He
> suggested that you modify the process just a little by asking them to send
> you the info either by fax or snail mail (or call you and give it to you
> over the phone). Hopefully it won't be a problem to make that minor change.

Can some provide the exact wording that would like for this and I will get it added to the form.

dkl

Comment 44

[David Lawrence [:dkl]]

(In reply to comment #43)
> (In reply to comment #42)
> > William and Pierros - With respect to how reps send William their bank wire
> > info, Alex agreed with Dave that sending by email is too insecure. He
> > suggested that you modify the process just a little by asking them to send
> > you the info either by fax or snail mail (or call you and give it to you
> > over the phone). Hopefully it won't be a problem to make that minor change.
> 
> Can some provide the exact wording that would like for this and I will get
> it added to the form.
> 
> dkl

Ping? Is this still needed?

dkl

Comment 45

[Pierros Papadeas [:pierros]]

Hey dkl you can close this, as the process now included a password protected file send to accounting (when bank info is involved) for over a year now. Thanks!