Ahsan/removal underscore string from commerce js #12401

ahsan-ul-haq · 2016-05-09T11:26:26Z

ECOM-4281
@awais786 @zubair-arbi @tasawernawaz @waheedahmed

Add js test for analytics event Completed Order on LMS checkout receipt page.
Remove underscore.string dependency and use global string_utils methods.

Initial PR (Merged): https://github.com/edx/edx-platform/pull/12238
Reverted PR (falkytest): https://github.com/edx/edx-platform/pull/12273
Related PR (Underscore string changes): https://github.com/edx/edx-platform/pull/11631

ahsan-ul-haq · 2016-05-09T12:11:33Z

jenkins run a11y

ahsan-ul-haq · 2016-05-09T12:12:36Z

jenkins run python
jenkins run bokchoy

ahsan-ul-haq · 2016-05-09T13:46:52Z

jenkins run bokchoy

ahsan-ul-haq · 2016-05-09T14:19:34Z

@andy-armstrong @bjacobel @mattdrayer I have fixed the JS issue regarding _sprintf which we missed previously for non verified course receipts. @mattdrayer can you please check this on your sandbox.

andy-armstrong · 2016-05-09T14:28:59Z

@ahsan-ul-haq Thanks for tackling this again, and for adding a regression test. There seems to be some issue where the test doesn't get full code coverage:

https://build.testeng.edx.org/job/edx-platform-js-pr/18172/Diff_Coverage_Report/

Can you investigate whether this is because the test isn't actually running, or if there's an issue with the code coverage itself. I'd expect to see the spec file get 100% coverage, and then the covered file should also get substantially more than 16.7%

ahsan-ul-haq · 2016-05-10T11:36:55Z

@andy-armstrong I have made the changes and now spec file got 100% of coverage and covered file got 33.3% coverage.

rlucioni · 2016-05-10T14:00:36Z

@ahsan-ul-haq you may want to hold off on merging this until https://github.com/edx/edx-platform/pull/12413 lands. The release branch contains a revert of the original commit. If you were to merge this before the revert lands in master, it might revert changes in this PR. @andy-armstrong and I learned about this the hard way a few releases back.

andy-armstrong · 2016-05-10T14:03:13Z

lms/static/js/fixtures/commerce/checkout_receipt.html

+    <section class="wrapper carousel">
+        <div id="receipt-container" class="pay-and-verify hidden" data-is-payment-complete='True'
+             data-platform-name='edx-platform' data-verified='True' data-username='user-1'>
+            <h2>${_("Loading Order Data...")}</h2>


Nit: the fixture is intended to be rendered HTML, so it shouldn't include Python in it. It won't hurt as long as it is valid HTML, but it is a little confusing.

ahsan-ul-haq · 2016-05-10T14:03:24Z

@rlucioni thanks for the heads up on that I would keep a close look on release to get merged into master before moving forward with this.

andy-armstrong · 2016-05-10T14:06:20Z

lms/static/js/spec/commerce/receipt_spec.js

+                    "currency": "USD",
+                    "total_excl_tax": "10.00"
+                };
+                view = new ReceiptView({el: $('#receipt-container')});


There's a subtle issue where events sometimes don't behave as you'd expect when you render views in the beforeEach method. This has been the cause of a number of flaky Jasmine tests. I'd convert this into a createTestView function, and then call it from your test to create a view.

I also find that it makes the test easier to read. I like tests to be of the form:

do a thing

verify the result

do a thing

verify the result

IMO having too much logic in the beforeEach makes it harder to read the test by itself without having to fully understand what the setup code is doing.

andy-armstrong · 2016-05-10T14:13:28Z

@ahsan-ul-haq Thanks for solving the issue with the spec coverage. Can you also get your changes to 100% coverage (this is just for lines you changed, not for the whole file):

https://build.testeng.edx.org/job/edx-platform-js-pr/18244/Diff_Coverage_Report/

You just need tests for getReceiptData and getProviderData which is a good idea anyway just to be sure that the interpolation is working as expected.

andy-armstrong · 2016-05-10T14:14:31Z

@ahsan-ul-haq This is looking great. I have a few more comments about the tests, so let me know when those are addressed.

ahsan-ul-haq · 2016-05-11T15:08:01Z

jenkins run js

zubair-arbi · 2016-05-12T11:00:41Z

lms/static/js/spec/commerce/receipt_spec.js

+                return new ReceiptView({el: $('#receipt-container')});
+            };
+
+            var loadReceiptFixture = function(){


Add space between () and {, like var loadReceiptFixture = function() {
Also add this var at top of this method.

ahsan-ul-haq · 2016-05-12T11:49:44Z

@andy-armstrong I have added some new tests and although tests passes I am getting following error afterward

Firefox 28.0.0 (Ubuntu 0.0.0) ERROR
  TypeError: text is null
  at /edx/app/edxapp/edx-platform/lms/static/common/js/vendor/underscore.js:1429
Firefox 28.0.0 (Ubuntu 0.0.0) ERROR
  TypeError: text is null

Do you have any idea about this?

zubair-arbi · 2016-05-12T12:01:02Z

once tests fixed and build passes 👍

ahsan-ul-haq · 2016-05-12T13:00:32Z

jenkins run bokchoy

andy-armstrong · 2016-05-12T14:34:59Z

lms/static/js/spec/commerce/receipt_spec.js

+
+            });
+
+            it("test render receipt", function() {


Nit: the test name should read like a sentence starting with it, so something like:

it('renders a receipt correctly', function() {

andy-armstrong · 2016-05-12T14:40:08Z

@ahsan-ul-haq That line of Underscore.js appears to be in the function _.template, so it seems that somewhere it is being rendered with a null template. I recommend putting a breakpoint on that line and then running the tests inside the browser (if you haven't done that before, this is described here: https://github.com/edx/edx-platform/blob/master/docs/en_us/internal/testing.rst#running-javascript-unit-tests).

ahsan-ul-haq · 2016-05-13T13:31:58Z

@andy-armstrong please review

ahsan-ul-haq · 2016-05-13T18:07:02Z

@AlasdairSwan please review

AlasdairSwan · 2016-05-13T18:19:50Z

lms/static/js/spec/commerce/receipt_spec.js

+                 AjaxHelpers.respondWithJson(requests, responseData);
+            };
+
+            var mockRender = function() {


There should only be 1 var declaration at the top of the function, comma separating the variable names.

ahsan-ul-haq · 2016-05-16T03:51:13Z

@AlasdairSwan @andy-armstrong made the suggested changes please review.

jbzdak · 2016-05-16T10:15:57Z

@ahsan-ul-haq First of all thanks for this work! I'm working on a small change to the receipt_view.js, and would like to base tests for my PR on your work.

I noticed following behavior when running tests for this view on my devstack: when I run whole lms test suite (paver test_js_dev -s lms and then just ran all the tests on http://192.168.33.10:9876/debug.html) everything works, however when I run only this suite (by going to: http://192.168.33.10:9876/debug.html?spec=edx.commerce.ReceiptView) I get test failures.

Could you please verify that is the case? If so I think that this would be worth fixing, as it may suggest some inter-dependencies between tests that could lead to sudden test failures in the future.

Example stack-trace looks like that:

Error: Expected 1 to be greater than 1.
    at stack (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1577:17)
    at buildExpectationResult (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1547:14)
    at Spec.Env.expectationResultFactory (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:638:18)
    at Spec.addExpectationResult (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:330:34)
    at Expectation.addExpectationResult (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:588:21)
    at Expectation.toBeGreaterThan (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1501:12)
    at currentRequest (http://192.168.33.10:9876/base/common/js/spec_helpers/ajax_helpers.js:71:33)
    at Object.expectRequest (http://192.168.33.10:9876/base/common/js/spec_helpers/ajax_helpers.js:76:23)
    at mockRequests (http://192.168.33.10:9876/base/js/spec/commerce/receipt_spec.js:36:30)
    at mockRender (http://192.168.33.10:9876/base/js/spec/commerce/receipt_spec.js:49:17)
TypeError: Cannot read property 'readyState' of undefined
    at Object.expectRequest (http://192.168.33.10:9876/base/common/js/spec_helpers/ajax_helpers.js:77:23)
    at mockRequests (http://192.168.33.10:9876/base/js/spec/commerce/receipt_spec.js:36:30)
    at mockRender (http://192.168.33.10:9876/base/js/spec/commerce/receipt_spec.js:49:17)
    at Object.<anonymous> (http://192.168.33.10:9876/base/js/spec/commerce/receipt_spec.js:149:17)
    at attemptSync (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1886:24)
    at QueueRunner.run (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1874:9)
    at QueueRunner.execute (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1859:10)
    at Spec.Env.queueRunnerFactory (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:697:35)
    at Spec.execute (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:359:10)
    at Object.fn (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:2479:37)

ahsan-ul-haq · 2016-05-16T13:51:40Z

@jbzdak I have tried to investigate/fix this but could not find any inter-dependencies between the tests. If you could find any please let me know.

AlasdairSwan · 2016-05-16T15:09:11Z

lms/static/js/spec/commerce/receipt_spec.js

+        'jquery', 'jquery.ajax-retry',
+        'js/commerce/views/receipt_view', 'common/js/spec_helpers/ajax_helpers'
+    ],
+    function ($, AjaxRetry, ReceiptView, AjaxHelpers){


Where is AjaxRetry being used?

because of https://github.com/edx/edx-platform/pull/12401/files/74d697952d7004aa715df532def3ffdbc255bd37#diff-51cca05dc2d297d186cdc94054145f1eR153

So it's not being used in this file, it's being used in lms/static/js/commerce/views/receipt_view.js? If so it should be in that file's define array.

There is no define array in lms/static/js/commerce/views/receipt_view.js.

Ahhh, I see now. Sorry, the function statement at the top made me think there was.

andy-armstrong · 2016-05-16T15:14:44Z

👍

AlasdairSwan · 2016-05-16T15:45:36Z

👍

andy-armstrong · 2016-05-16T16:17:48Z

@jbzdak @ahsan-ul-haq I've pushed up a fix for running the tests individually. I stepped through the code and found that it was silently throwing an error trying to record events for analytics. I added the standard code for adding spy methods for the expected analytics functions. I also renamed the spec file to mirror the file it was testing, and cleaned up the order of spec files in the Karma main.js file.

andy-armstrong · 2016-05-16T16:22:25Z

@jbzdak @ahsan-ul-haq I should have mentioned that there are many tests that are not clearing the analytics spy once they are done, which is why running all the tests together allowed these tests to run. Ideally we would clean up this logic so that the spies are removed after each test is run, and also we would share the initialization. FYI @jzoldak @benpatterson.

jzoldak · 2016-05-16T17:11:15Z

@andy-armstrong ugh. definitely thanks for noticing that, tho. Can you please make sure that there is a JIRA ticket for addressing the potential issues with the non-cleaned-up analytics spy?

jbzdak · 2016-05-16T19:11:16Z

@ahsan-ul-haq @andy-armstrong Thanks, that really helped a lot! When I run the tests one by one they run well.

However, on my devstack, when I run the whole fixture together (for example by this: http://192.168.33.10:9876/debug.html?spec=edx.commerce.ReceiptView) I still get a failure in: "sends analytic event when non verified receipt is rendered", this failure is related to view not making request to fetch the order (stacktrace is at the end of comment). I was able to debug it somewhat and I found the following:

RecepitView doesn't send request as this condition is false: this.$el.data('is-payment-complete') === 'True' (in render() function)
This condition is false as this.$el is empty, which is due to #receipt-container missing from the DOM.
This is missing from the DOM as readFixtures('js/fixtures/commerce/non_verified_receipt.html'); returns an empty string.
When I debugged readFixtures it turned out that for some reason during the call window.XMLHttpRequest is still set to FakeXMLHttpRequest from the the sinon library. It looks like in this case AjaxUtils didn't properly detach the request.

Error stacktrace is here: 
Error: Expected 0 to be greater than 0.
    at stack (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1577:17)
    at buildExpectationResult (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1547:14)
    at Spec.Env.expectationResultFactory (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:638:18)
    at Spec.addExpectationResult (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:330:34)
    at Expectation.addExpectationResult (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:588:21)
    at Expectation.toBeGreaterThan (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1501:12)
    at currentRequest (http://192.168.33.10:9876/base/common/js/spec_helpers/ajax_helpers.js:71:33)
    at Object.expectRequest (http://192.168.33.10:9876/base/common/js/spec_helpers/ajax_helpers.js:76:23)
    at mockRequests (http://192.168.33.10:9876/base/js/spec/commerce/receipt_view_spec.js:36:30)
    at mockRender (http://192.168.33.10:9876/base/js/spec/commerce/receipt_view_spec.js:54:17)
TypeError: Cannot read property 'readyState' of undefined
    at Object.expectRequest (http://192.168.33.10:9876/base/common/js/spec_helpers/ajax_helpers.js:77:23)
    at mockRequests (http://192.168.33.10:9876/base/js/spec/commerce/receipt_view_spec.js:36:30)
    at mockRender (http://192.168.33.10:9876/base/js/spec/commerce/receipt_view_spec.js:54:17)
    at Object.<anonymous> (http://192.168.33.10:9876/base/js/spec/commerce/receipt_view_spec.js:174:17)
    at attemptSync (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1886:24)
    at QueueRunner.run (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1874:9)
    at QueueRunner.execute (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1859:10)
    at Spec.Env.queueRunnerFactory (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:697:35)
    at Spec.execute (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:359:10)
    at Object.fn (http://192.168.33.10:9876/absolute/edx/app/edxapp/edx-platform/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:2479:37)

andy-armstrong · 2016-05-16T19:25:40Z

@jzoldak I've created a story to track the Jasmine spy issue:

https://openedx.atlassian.net/browse/TNL-4582

ahsan-ul-haq · 2016-05-16T19:46:11Z

@andy-armstrong Thanks for catching this

ahsan-ul-haq · 2016-05-17T12:34:42Z

@jbzdak I have made the changes for individual tests to run in debug mode and now its working fine on my local devstack. Please confirm so I can move forward with merging this.

ahsan-ul-haq · 2016-05-17T13:30:25Z

jenkins run bokchoy

ECOM-4281

andy-armstrong · 2016-05-17T16:27:18Z

@mattdrayer This is the PR that is causing you trouble with the merge back.

andy-armstrong · 2016-05-17T16:28:49Z

Here is the PR that is merging the release back to master:

https://github.com/edx/edx-platform/pull/12495

We will have to be careful not to lose any of this newer PR with the merge.

andy-armstrong reviewed May 10, 2016
View reviewed changes

ahsan-ul-haq force-pushed the ahsan/removal-underscore-string-from-commerce-js branch from 36a15eb to 7a0e249 Compare May 11, 2016 15:52

zubair-arbi reviewed May 12, 2016
View reviewed changes

andy-armstrong reviewed May 12, 2016
View reviewed changes

ahsan-ul-haq force-pushed the ahsan/removal-underscore-string-from-commerce-js branch from bfba4a6 to 6d41a0d Compare May 12, 2016 15:56

AlasdairSwan reviewed May 13, 2016
View reviewed changes

ahsan-ul-haq force-pushed the ahsan/removal-underscore-string-from-commerce-js branch from 48d8661 to 026c739 Compare May 14, 2016 22:44

jbzdak mentioned this pull request May 16, 2016

Honor verification required setting for professional courses in the receipt view #12470

Merged

2 tasks

AlasdairSwan reviewed May 16, 2016
View reviewed changes

ahsan-ul-haq force-pushed the ahsan/removal-underscore-string-from-commerce-js branch from 24daa42 to af7eb1f Compare May 16, 2016 20:06

Add js test for analytics event on LMS receipt page

300002b

ECOM-4281

ahsan-ul-haq force-pushed the ahsan/removal-underscore-string-from-commerce-js branch from be7b2d7 to 300002b Compare May 17, 2016 13:56

ahsan-ul-haq merged commit 7a43b57 into master May 17, 2016

ahsan-ul-haq deleted the ahsan/removal-underscore-string-from-commerce-js branch May 17, 2016 14:49

Ahsan/removal underscore string from commerce js #12401

Ahsan/removal underscore string from commerce js #12401

Conversation

ahsan-ul-haq commented May 9, 2016 • edited

ahsan-ul-haq commented May 9, 2016

ahsan-ul-haq commented May 9, 2016

ahsan-ul-haq commented May 9, 2016

ahsan-ul-haq commented May 9, 2016

andy-armstrong commented May 9, 2016

ahsan-ul-haq commented May 10, 2016

rlucioni commented May 10, 2016

Choose a reason for hiding this comment

ahsan-ul-haq commented May 10, 2016

Choose a reason for hiding this comment

andy-armstrong commented May 10, 2016

andy-armstrong commented May 10, 2016

ahsan-ul-haq commented May 11, 2016

Choose a reason for hiding this comment

ahsan-ul-haq commented May 12, 2016

zubair-arbi commented May 12, 2016

ahsan-ul-haq commented May 12, 2016

Choose a reason for hiding this comment

andy-armstrong commented May 12, 2016

ahsan-ul-haq commented May 13, 2016

ahsan-ul-haq commented May 13, 2016

Choose a reason for hiding this comment

ahsan-ul-haq commented May 16, 2016

jbzdak commented May 16, 2016

ahsan-ul-haq commented May 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-armstrong commented May 16, 2016

AlasdairSwan commented May 16, 2016

andy-armstrong commented May 16, 2016

andy-armstrong commented May 16, 2016 • edited

jzoldak commented May 16, 2016

jbzdak commented May 16, 2016

andy-armstrong commented May 16, 2016

ahsan-ul-haq commented May 16, 2016

ahsan-ul-haq commented May 17, 2016 • edited

ahsan-ul-haq commented May 17, 2016

andy-armstrong commented May 17, 2016

andy-armstrong commented May 17, 2016

ahsan-ul-haq commented May 9, 2016 •

edited

andy-armstrong commented May 16, 2016 •

edited

ahsan-ul-haq commented May 17, 2016 •

edited