841202 - Refactor code in current MozProjectReview extension to be less complex and more maintainable

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozProjectReview, defect)

Description

[David Lawrence [:dkl]]

Currently the create form has quite a bit of embedded javascript that decides which sections to display of a form based on values of certain fields. Initially this was not an issue when the fields were few and there were only a few logic paths to worry about. This has recently not scaled very well and the code needs to be refactored to be easier to extend and maintain.

The plan is to create a single javascript function that is hooked to each of the fields that decide visibility of other sections. Inside the single function, we have a table of visible sections which start out as off by default. Then we iterate through each field one by one, look at it's value and decide which sections should be visible based on it's current value. Then at the end of the function, loop over the visible sections table and turn off/on the section based on its current true/false value. 

The logic will be based off of the excellent chart that mcoates has created and will allow use to more easily compare the chart to the actual code rather that having to go through the form manually as I do currently.

dkl

Comment 1

[Michael Coates [:mcoates] (acct no longer active)]

I propose we link this major change with our version 1.1 deployment which will accompany multiple changes and receive significant testing.

Comment 2

[David Lawrence [:dkl]]

I did most of this work on the plane up to London and is ready for review and testing. Basically it has same functionality as the current production form but with the fully refactored handling of the sections/questions matching the current flowchart.

Please take a look when you can
https://bugzilla-stage-tip.mozilla.org/form.moz.project.review

dkl

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to refactor handling of the project review form (v1)

Low priority. Just wanted to get this out for your feedback on my code changes.

Thanks
dkl

Comment 4

[:glob ✱]

Comment on attachment 716475 [details] [diff] [review]
Patch to refactor handling of the project review form (v1)

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

looks good; i can't really determine if the logic is correct, but that should fall out of the testing :)

there's trailing whitespace which needs to be removed, and a few other nits:

::: extensions/MozProjectReview/web/js/moz_project_review.js
@@ +32,5 @@
> +            "finance_purchase_cost": "Please enter a value for total cost in the finance questions section"
> +        },
> +        "legal_questions": {
> +            "legal_priority": "Please select a priority for the legal questions section",
> +            "legal_help_from_legal": "Please describe the help needed from the Legal department",

remove the trailing , from the last item in this array, some js implementations don't like it.

@@ +48,5 @@
> +            "legal_sow_vendor_payment": "Please enter a value for SOW vendor payment amount",
> +            "legal_sow_vendor_payment_basis": "Please enter a value for SOW vendor payment basis",
> +            "legal_sow_vendor_payment_schedule": "Please enter a value for SOW vendor payment schedule",
> +            "legal_sow_vendor_total_max": "Please enter a value for SOW vendor maximum total to be paid",
> +            "legal_sow_vendor_product_line": "Please enter a value for SOW vendor product line",

another trailing , to remove

@@ +77,5 @@
> +        'finance_purchase_urgency',
> +        'data_safety_user_data',
> +        'data_safety_retention',
> +        'data_safety_separate_party',
> +        'data_safety_community_visibility',

another trailing , to remove

@@ +115,5 @@
> +            data_safety_extra_questions: false,
> +            data_safety_retention_length_row: false,
> +            data_safety_separate_party_data_row: false,
> +            data_safety_communication_channels_row: false,
> +            data_safety_communication_plan_row: false,

another trailing , to remove

@@ +139,5 @@
> +            page_sections.initial_separate_party_questions = true;
> +
> +            if (Dom.get('relationship_type').value
> +                && Dom.get('relationship_type').value != 'Hardware Purchase')
> +                page_sections.legal_questions = true;

the indentation here doesn't clearly separate the condition from the operation.  perhaps { braces } would work better (or simply don't break the condition into two lines).

@@ +207,5 @@
> +    toggleShowSection: function (section, show) {
> +        if (show)
> +            Dom.removeClass(section, 'bz_default_hidden');
> +        else 
> +            Dom.addClass(section ,'bz_default_hidden');

use { braces } for if/else statements.

@@ +246,5 @@
> +
> +        if (Dom.get('separate_party').value == 'Yes') {
> +            if (!MPR.isFilledOut('relationship_type')) alert_text += "Please select a value for type of relationship\n";
> +            if (!MPR.isFilledOut('data_access')) alert_text += "Please select a value for data access\n";
> +            if (!MPR.isFilledOut('vendor_cost')) alert_text += "Please select a value for vendor cost\n";

to match the rest of the file, don't fold these if/then onto a single line.

Comment 5

[David Lawrence [:dkl]]

Thanks glob.

Mcoates? Would you or someone be able to test the form out on our test 4.2 instance this week and let me know how it looks? If so, then I can go ahead and commit to our test instance today and then you can test it and it will be live when we upgrade beginning of next week.

https://bugzilla.allizom.org (after i commit it)

If not, we can roll it out sometime after the upgrade.

Thanks
dkl

Comment 6

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #5)
> Thanks glob.
> 
> Mcoates? Would you or someone be able to test the form out on our test 4.2
> instance this week and let me know how it looks? If so, then I can go ahead
> and commit to our test instance today and then you can test it and it will
> be live when we upgrade beginning of next week.
> 
> https://bugzilla.allizom.org (after i commit it)
> 
> If not, we can roll it out sometime after the upgrade.
> 
> Thanks
> dkl

Michael. Actually glob and I discussed this and we will just roll out the refactor soon after the 4.2 upgrade and not commit it prior as this will increase the chances of a successful upgrade. I have update the patch on our current 4.0 test instance so please try it out there.

https://bugzilla-stage-tip.mozilla.org/form.moz.project.review

Also I am not sure that this should be held back by the other enhancements being requested for version 1.1 of the form as this refactoring mimics the current behavior if the form and does not add anything to it. Basically this is a patch to write the form in the way it should have been done initially once I realized the old way was not maintainable. So I feel that once we have verified everything works the way it should be should push it out as soon as we can.

Thanks
dkl

Comment 7

[Michael Coates [:mcoates] (acct no longer active)]

When selecting yes for #11 it is (incorrectly) not displaying Privacy Technical questions. Note: I think this may be an outstanding bug in our production version too, but let's just fix it here.

See pg 3 (v1.0) of flow document
https://docs.google.com/a/mozilla.com/file/d/0B2Lct3lOMM6ZeTd3S3ZvMGxPbk0/edit?usp=sharing

Comment 8

[Michael Coates [:mcoates] (acct no longer active)]

(In reply to Michael Coates [:mcoates] from comment #7)
> When selecting yes for #11 it is (incorrectly) not displaying Privacy
> Technical questions. Note: I think this may be an outstanding bug in our
> production version too, but let's just fix it here.
> 
> See pg 3 (v1.0) of flow document
> https://docs.google.com/a/mozilla.com/file/d/0B2Lct3lOMM6ZeTd3S3ZvMGxPbk0/
> edit?usp=sharing

Nevermind - I didn't look at pg 4 (we don't actually have any questions for privacy technical). So we're good so far.

Comment 9

[Michael Coates [:mcoates] (acct no longer active)]

Something failed on submission. Note, on my first submission I got a warning that I had a bad failure "Points of Contact". I submitted a non-valid name. I hit the back button, corrected it, and resubmitted. Not sure if that is related to the problem.


https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=686956

Bug 686957 - Security Review: mc-test1
Error creating legal review bug
Bug 686958 - Privacy-Technical Review: mc-test1
Bug 686959 - Privacy-Policy Review: mc-test1

Comment 10

[David Lawrence [:dkl]]

(In reply to Michael Coates [:mcoates] from comment #9)
> Something failed on submission. Note, on my first submission I got a warning
> that I had a bad failure "Points of Contact". I submitted a non-valid name.
> I hit the back button, corrected it, and resubmitted. Not sure if that is
> related to the problem.

Probably not relate to the points of contact as it should be fine to back up and fix the value and resubmit.

> https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=686956
> 
> Bug 686957 - Security Review: mc-test1
> Error creating legal review bug
> Bug 686958 - Privacy-Technical Review: mc-test1
> Bug 686959 - Privacy-Policy Review: mc-test1
'
I think I see the issue with the Legal Bug. In the code if "New or Change" with a value of 'Neither' is not setting the Legal bugs component properly. For a value of 'New" I am defaulting to "General' for component and for 'Existing' I am use the value for 'Mozilla Project' that becomes visible.

What should the component be for the Legal bug when 'Neither' is selected for 'New or Change'?

dkl

Comment 11

[Michael Coates [:mcoates] (acct no longer active)]

What should the component be for the Legal bug when 'Neither' is selected for 'New or Change'?

General

Comment 12

[David Lawrence [:dkl]]

(In reply to Michael Coates [:mcoates] from comment #11)
> What should the component be for the Legal bug when 'Neither' is selected
> for 'New or Change'?
> 
> General

I have pushed this change for testing.

https://bugzilla-stage-tip.mozilla.org/form.moz.project.review

dkl

Comment 13

[Michael Coates [:mcoates] (acct no longer active)]

All looks good. Let's push this to prod.

Comment 14

[David Lawrence [:dkl]]

Thanks. 

Now that this is checked in, all further testing can be done at

https://bugzilla-dev.allizom.org (our internal dev instance)
or
https://bugzilla.allizom.org (staging instance)

Committing to: bzr+ssh://[e-mail]/bmo/4.2         
modified extensions/MozProjectReview/Extension.pm
modified extensions/MozProjectReview/template/en/default/bug/create/create-moz-project-review.html.tmpl                                                                  
modified extensions/MozProjectReview/web/js/moz_project_review.js
Committed revision 8643. 

dkl