Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add js test for analytics event on LMS receipt page #12290

Merged

Conversation

zubair-arbi
Copy link
Contributor

@zubair-arbi zubair-arbi commented Apr 28, 2016

ECOM-4281
@awais786 @ahsan-ul-haq @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

@zubair-arbi
Copy link
Contributor Author

jenkins run js
jenkins run lettuce

/* Mix non-conflicting functions from underscore.string (all but include, contains, and reverse) into
* the Underscore namespace.
*/
_.mixin(_s.exports());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy-armstrong @AlasdairSwan Including following code

'js/commerce/views/receipt_view': {
    exports: 'edx.commerce.ReceiptView',
    deps: ['jquery', 'backbone', 'underscore', 'underscore.string'],
    init: function() {
        // Set global variables that the receipt code is expecting to be defined
        require([
            'underscore',
            'underscore.string'
        ], function (_, str) {
            window._ = _;
            window._s = str;
        });
    }
 },

in the file lms/static/js/spec/main.js doesn't help resolve the error (for js spec test):

Firefox 28.0.0 (Ubuntu 0.0.0) ERROR
  TypeError: _.str is undefined
  at /edx/app/edxapp/edx-platform/lms/static/js/commerce/views/receipt_view.js:28

while doing _.mixin(_.str.exports());

Is there is something we are missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also add:

window._.str = str;

I suppose.

Note that we are working to get rid of Underscore.string because it is mostly not needed with the latest browsers. My recommendation would be to remove this line, and to try to remove any usages of it in your code. @bjacobel might be able to offer some advice as he's worked on this recently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy-armstrong Here we have removed _.str usage and also instead of using _.sprintf which is method of _.str we have manually formatted the urls. So do you think it is a good enough solution for now or would you like to suggest some other solution.
FYI: @bjacobel, @symbolist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on what utils in Underscore.string you're using, the UI Toolkit's StringUtils may fill your needs. Just looking through this quickly seems like you're just using _.sprintf, which can be replaced by StringUtils.interpolate easily.

Here's an example of a file I removed sprintf's from a few weeks ago:

@ahsan-ul-haq
Copy link
Contributor

jenkins run bokchoy

@zubair-arbi zubair-arbi force-pushed the zub/ecom-4281-add-receipt-page-analytics-event-js-test branch from 0652b38 to 3271d07 Compare April 29, 2016 11:18
@zubair-arbi
Copy link
Contributor Author

@AlasdairSwan @schenedx @bjacobel I have used global method interpolate_text instead of using underscore.string method _.sprintf.
Please review.

@zubair-arbi
Copy link
Contributor Author

@rlucioni It would be helpful if you can review too.

@@ -3,7 +3,7 @@
*/
var edx = edx || {};

(function ($, _, _s, Backbone) {
(function ($, _, Backbone, interpolate_text) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing the Underscore.string usages. This wasn't clear, but interpolate_text is being deprecated in favor of StringUtils.interpolate and HtmlUtils.interpolateHtml. In your case, use edx.StringUtils.interpolate as this code is not using RequireJS.

See the safe template guidelines here:

http://edx-developer-guide.readthedocs.io/en/latest/conventions/safe_templates.html

@ahsan-ul-haq ahsan-ul-haq force-pushed the zub/ecom-4281-add-receipt-page-analytics-event-js-test branch from 7713148 to f995114 Compare May 2, 2016 08:15
@ahsan-ul-haq
Copy link
Contributor

@andy-armstrong @AlasdairSwan The PR is ready could you give your final thoughts about it.
FYI: @bjacobel

@AlasdairSwan
Copy link
Contributor

Looks good 👍

@ahsan-ul-haq
Copy link
Contributor

@andy-armstrong waiting for your review.

@andy-armstrong
Copy link
Contributor

👍

@ahsan-ul-haq ahsan-ul-haq merged commit dde18c7 into master May 2, 2016
@ahsan-ul-haq ahsan-ul-haq deleted the zub/ecom-4281-add-receipt-page-analytics-event-js-test branch May 2, 2016 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants