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

Fixes #8020

It's the same as https://github.com/piwik/piwik/pull/8205 but excluding changes in /tests/javascript/index.php and fixed widgetMenu.js.

@tsteur commented on June 29th 2015 Member

I tested them seems to work apart from one, nice!

  • Remove dashboard seems to not work anymore (Chrome 43 on Mac)

I have a few comments but they are not necessarily related to this PR, I'm not sure. We could create a follow up issue for this. Same of those issues might have existed before.

  • Eg when going to "Admin => General settings" and clicking "Save" (or Sites Management add/edit site or Users Management edit user) it could be nice to change the "state" of the button and use a wording like "Saving" and making the button disabled while the saving is in progress. We recently changed this in the Core Updater as one didn't know whether it is actually doing something or not. It was not really a big issue before as the requests were sync and one couldn't do anything while they were in progress
  • When clicking "Save" in "Admin => Plugin settings" it shows "Loading data..." above the "Save" button for a short time. When deleting a user it shows "Loading data..." for a short time as well. Also when pressing "Save" in Personal Settings.

I noticed some more problems but they seem not related to this change: #8245 #8246 #8247

@tsteur commented on June 29th 2015 Member

It's nice to have everything async!

@barbushin commented on June 30th 2015 Contributor

@tsteur Thank you for that review! I removed deprecated methods and notices. Also I checked #8245 and #8247 and all of them was reproduced on master, so it looks like they are not relative to this Pull request. Anyway, I submited pull requests with fixes for this issues.

@tsteur commented on July 1st 2015 Member

Sweet. Did you have a look re removing a dashboard? Does that work for you?
I will create issues for my other 2 points as they are a problem in general and not really relate to this PR

@barbushin commented on July 2nd 2015 Contributor

Do you mean remove widget on Dashboard?
image Looks like it works without any bugs.

@tsteur commented on July 2nd 2015 Member

Sorry I meant removing an actual dashboard. This can be done in the "Dashboard & Widgets" selector in the bottom left.

@barbushin commented on July 3rd 2015 Contributor

I've created new dashboard, and removed, and it works http://i.imgur.com/y9Hs1RF.gif

@tsteur commented on July 5th 2015 Member

For some reason it works for me too now. Cannot reproduce it anymore... Good to merge after 2.14.0 release

@tsteur commented on July 6th 2015 Member

Actually the Dashboard UI tests are failing, maybe because of this change?

@mattab commented on July 12th 2015 Member

Actually the Dashboard UI tests are failing, maybe because of this change?

I've restarted the build to check this can be reproduced. If the tests still fail then it's an issue either with the UI tests, or the PR?

@barbushin commented on July 13th 2015 Contributor

I don't undestand how to see/reproduce UI tests failure. I see links in UI tests results:

View UI failures (if any) here:
or: http://builds-artifacts.piwik.org/ui-tests.master/13864.7/screenshot-diffs/diffviewer.html

But it does not works.

@mattab commented on July 13th 2015 Member

@barbushin it looks like our screenshot uploader / UI test viewer is buggy for pull requests done in branches outside of Piwik/piwik (it works fine for PR done by core team members).
can you create a bug report here so we don't forget to fix this issue?

To workaround, could you try to re-create PR on the piwik/piwik branch and then maybe it works then?

Btw are you able to run UI tests locally and reproduce the UI test failure locally?

@tsteur commented on July 13th 2015 Member

FYI I created https://github.com/piwik/ui-tests-viewer/issues/11

It should be good to merge but can't see why tests fail. Should ideally only merge when tests are green.

@barbushin commented on July 20th 2015 Contributor

@mattab I re-created PR from piwik/8020_xhr_sync_fix branch. There are same UI Tests failing, but it looks like the problem is not in PR code change. Please take a loot at the comments there https://github.com/piwik/piwik/pull/8381#issuecomment-122835011

@mattab commented on August 13th 2015 Member

Duplicate of #8381

This Pull Request was closed on August 13th 2015
Powered by GitHub Issue Mirror