Status

()

bugzilla.mozilla.org
General
--
enhancement
RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: glob, Assigned: dkl)

Tracking

Production

Details

Attachments

(4 attachments)

(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?
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.
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.
s/date/data/ 

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

Comment 4

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

Comment 5

7 years ago
Jim says they should send their reimbursement requests to mozilla@bill.com.
(Assignee)

Comment 6

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

dkl
a standalone app makes more sense, and we already have one (bill.com).
marking as wontfix as per comment 5.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
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?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 9

7 years ago
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.
Thanks Liz, William has already planned a meeting with accounting today, and I will update the bug accordingly.
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.
(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.
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. :(
:( .. 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

7 years ago
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

7 years ago
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?
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → WONTFIX
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.
Ping, because this is a blocker for Mozilla Reps program to start, do we have any ETA?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 19

7 years ago
(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?
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!
(Assignee)

Comment 21

7 years ago
Working on this.
Assignee: nobody → dkl
Status: REOPENED → ASSIGNED
(Assignee)

Comment 22

7 years ago
Created attachment 535485 [details] [diff] [review]
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
Attachment #535485 - Flags: review?(glob)
(Assignee)

Comment 23

7 years ago
pierros, you can give this a try at 

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

Dkl
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 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 <gerv@gerv.net>
> +  #%]

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 <reed@mozilla.com>
> +  #                 David Tran <dtran@mozilla.com>

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/>
Attachment #535485 - Flags: review?(glob) → review-
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.
(Assignee)

Comment 27

7 years ago
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
(Assignee)

Comment 28

7 years ago
Created attachment 535721 [details] [diff] [review]
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
Attachment #535721 - Flags: review?(glob)
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' %]
(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 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.
Attachment #535721 - Flags: review?(glob) → review-
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
(Assignee)

Comment 33

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

dkl
(Assignee)

Comment 34

7 years ago
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
Attachment #535721 - Flags: review- → review?(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
Attachment #535721 - Flags: review?(glob) → review-
(Assignee)

Comment 36

7 years ago
Created attachment 536232 [details] [diff] [review]
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
Attachment #536232 - Flags: review?(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.
Attachment #536232 - Flags: review?(glob) → review-
(Assignee)

Comment 38

7 years ago
Created attachment 536315 [details] [diff] [review]
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
Attachment #536315 - Flags: review?(glob)
Comment on attachment 536315 [details] [diff] [review]
Patch to create payment form (v4)

r=glob :)
Attachment #536315 - Flags: review?(glob) → review+
(Assignee)

Comment 40

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

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

dkl
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 41

7 years ago
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."
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 42

7 years ago
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.
(Assignee)

Comment 43

7 years ago
(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
(Assignee)

Updated

7 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 44

7 years ago
(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
(Assignee)

Updated

5 years ago
Flags: needinfo?(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!
Flags: needinfo?(pierros)
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.