@barbushin opened this Pull Request on June 24th 2015 Contributor

Fixes #8020

Also I did not refactored sync XHR request in /tests/javascript/index.php:setupContentTrackingFixture() because there was too much problems with qUnit handling async calls in tests.

@barbushin commented on June 24th 2015 Contributor

There was 2 tests failing before this merge http://i.imgur.com/lw8hoYS.png and after all changes JS tests are still failing only on this 2. I think they should be fixed it in some another issue.

@barbushin commented on June 24th 2015 Contributor

There is also relative pull request for CustomAlerts plugin https://github.com/piwik/plugin-CustomAlerts/pull/17

@tsteur commented on June 26th 2015 Owner

I'm not sure if this one is for review but already added a few comments. We definitely need to mention the deprecation of sync requests in the changelog (/CHANGELOG.md) and ideally we create a follow up issue to remove it in Piwik 3.0

We will definitely need to carefully test each change manually and check for possible race conditions etc. Eg sometimes we might need to cancel another already running XHR before requesting a new one or we need to make sure that another request is finished before executing another one. Might make sense to use $q library in UI as it won't be realistic to rewrite everything to angular.

Also I did not refactored sync XHR request in /tests/javascript/index.php:setupContentTrackingFixture() because there was too much problems with qUnit handling async calls in tests.

That's absolutely okay

@barbushin commented on June 26th 2015 Contributor

@tsteur Please take a loot at https://github.com/barbushin/piwik/commit/1df9fcddda39bb50534ab84635325e6c5c745032 I've fixed widgetsHelper backward compatibility.

@barbushin commented on June 29th 2015 Contributor

I've created new Pull request https://github.com/piwik/piwik/pull/8242 with fixes in widgetMenu.js, and without any changes in /tests/javascript/index.php (so JS tests will still use sync calls).

@tsteur commented on June 29th 2015 Owner

I'm not sure but it shows quite a lot of "files changed"?

@barbushin commented on June 29th 2015 Contributor
@tsteur commented on June 29th 2015 Owner

Cheers! Haven't had a look at the other PR's yet. The other one works :)

This Pull Request was closed on June 29th 2015
Powered by GitHub Issue Mirror