1365658 - only allow integers for priority and backgroundRate

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (frontend), enhancement, P3)

Description

[A.S.]

Attachment: Server logs

While resolving [Bug 1336452](https://github.com/mozilla/balrog/pull/312 and https://bugzilla.mozilla.org/show_bug.cgi?id=1336452), the UI forms for posting rules (new or edit existing one) have two fields priority and backgroundRate defined as integer type in DB. However, up until before merging the PR above, the balrog's flask admin app was using wtfForms and its validations to fetch the values from the UI forms. However, after the merging the PR, wtfForms were deprecated and removed for the admin app's APIs. 

However, this exposed a previously unknown issue, it was found that the data passed from the UI clients was getting passed as all string (Suspected due to HTML forms have type as text and not integer only). Previously wtfForms were doing the implicit conversion from string to integer before passing the values for processing. 

A simple hack was made into the admin app code to overcome this problem. However, now this issue needs to be fixed.

Firstly, PFA Sample log in the attachments.They show how the request data was getting passed as string for priority and backgroundRate causing errors.

Here's where the balrog UI invokes admin app APIs : [https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/ui/app/js/services/rules_service.js#L30].
We need to verify whether priority and backgroundRate fields are integers before sending the request. If they aren't then throw appropriate error message by sweetAlert or a label like the one used here (https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/ui/app/templates/rule_modal.html#L164) . Best to discuss with the stake holders at the time before finalizing the change approach.


This is primarily a front-end issue.However, once these two fields are resolved on the front-End the backend changes/hacks can be simply removed or modified too.
Here's the list of changes that were done, which may be present in other API methods too.

1. Explicit int conversion:
 (https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/auslib/web/admin/views/rules.py#L65

2. priority format checker (verifies the string can be represented as int) [https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/auslib/web/admin/views/validators.py#L104]

3. backgroundRate format checker [https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/auslib/web/admin/views/validators.py#L110]

4. Change the type array to [integer,null] and specify the minimum - 0 and maximum- 100 for backgroundRate and only minimum-0 for priority in the swagger models. 

priority : https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/auslib/web/admin/swagger/api.yaml#L282

backgroundRate https://github.com/mozilla/balrog/blob/d6dc856d68919c155316ef132a73b4e57389a12c/auslib/web/admin/swagger/api.yaml#L293

Comment 1

[A.S.]

This same issue occurs for every integer field in Rules. It occurs for rule_id and data_version when they are passed in request body when duplicate existing rule form submits data.

Comment 2

[Daniel Kitui]

Hi. I would like to take a stab at this bug. Currently having an ind-epth look at the front-end points of concern and should be able to handle the back-end modifications too. Thank you

Comment 3

[Daniel Kitui]

This is the way I propose to resolve this issue:

1. Get rid of all the hacks that were in place in the 3 files that as spelled out in the bug description. This should bring back the captioned 500 server errors. == DONE.

2. Implement typecheck function in '/ui/app/js/controllers/rule_new_controller.js' for the values entered for priority and rate, and fire a sweetAlert error message that will highlight the problem values. == INPROGRESS.

I think using the sweetAlert message is more proactive and will catch a user's attention better than a div error label.

Your thoughts @Njira and @Ashish?

Comment 4

[Daniel Kitui]

Attachment: Screen Shot 2017-09-29 at 09.16.38.png

Screenshot of label error for invalid inputs.

Comment 5

[Daniel Kitui]

After going ahead and implementing using sweetAlerts, I feel that it's not the best UI. Having an alert pop out with the errors on only the two field is a bit odd, and counter-intuitive with the rest of the site. I, therefore, tried out the label error, as suggested by Ashish and I think it works best.

So implementation is still js validators for the two fields, and on the user clicking 'Save Changes', if there are any errors on the two fields, the input boxes are highlighted in red, and the specific error displayed below the respective inputs. This halts the function, and the API is not called untill the inputs are valid. See the screenshot attachment above.

Currently looking at the tests around this particular input validation to make sure they are adequate.

Comment 6

[[e-mail] (:bhearsum)]

Sounds like you're making good progress so far. I agree that using the error labels is the best place to show messages like this. Please keep in mind that there are many controllers that have these fields (new rule/edit rule/new rule scheduled change/edit rule scheduled change). If it's possible to centralize some of the logic (eg: with a helper or by putting into the rule service) that might be good. Otherwise, things may need to be duplicated into all of the affected controllers.

Comment 7

[Daniel Kitui]

Thank you for the feedback, and I will work on getting in helper functions to avoid the code duplication.

Comment 8

[Daniel Kitui]

@bhearsum Made the code more dry by including a validation method in '/ui/app/js/services/helper_service.js', that is used by the new rule/edit rule/new rule scheduled change/edit rule scheduled change controllers.

Also, I have restored the backend-hacks that already existed, as the second line of defence incase of any invalid values sneaking past UI validation, eg when JS is disabled(as informed by a stakeholder on my end).

Here is the meantime PR: https://github.com/mozilla/balrog/pull/415

Comment 9

[Daniel Kitui]

Updated PR: https://github.com/mozilla/balrog/pull/416

Comment 10

[Daniel Kitui]

PR updated again. https://github.com/mozilla/balrog/pull/416

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Daniel Kitui from comment #10)
> PR updated again. https://github.com/mozilla/balrog/pull/416

Responded there.

Comment 12

[Daniel Kitui]

Here is a new PR made after I moved my previous work to a new branch. This was to reduce the huge unrelated diff brought in by having to rebase to master twice. Its 

https://github.com/mozilla/balrog/pull/421

Comment 13

[Daniel Kitui]

(In reply to Daniel Kitui from comment #12)
> Here is a new PR made after I moved my previous work to a new branch. This
> was to reduce the huge unrelated diff brought in by having to rebase to
> master twice. Its 
> 
> https://github.com/mozilla/balrog/pull/421

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/f9b339743201e5fd325c4d7865d3ea258a83ff3f
bug 1365658: Bugfix add ui integer validation (#421). r=bhearsum

Comment 15

[[e-mail] (:bhearsum)]

This hit production today. Thank you Daniel!