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

Jump to bottom

Hide loading errors when user changes reporting page #17292

Merged
merged 5 commits into from Mar 14, 2021
Merged

Hide loading errors when user changes reporting page #17292

merged 5 commits into from Mar 14, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 2, 2021 •
edited

Description:

fixes #17104

fixes #10578

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz self-assigned this Mar 2, 2021
@flamisz flamisz added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Mar 2, 2021
sgiehl
sgiehl previously approved these changes Mar 3, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

I guess this makes it at least a bit better. But there are still some cases where an error message might be shown, that is unrelated. e.g. when you load a report page that sends xhr requests to fetch additional data. But before those requests were answered you already switch to another report page. If an old request then fails, the error message will be show even if it actually was for the page before. Guess that won't be that easy to fix, as we need some mechanism to automatically cancel xhr requests that aren't needed anymore (or at least ignore possible errors). Maybe we could store the url that was related to an XHR when it is sent or something similar. But guess that's something for another PR. ping @tsteur

@sgiehl sgiehl added this to the 4.5.0 milestone Mar 3, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 3, 2021

@sgiehl yes, you are right. I haven't thought about that. Let's wait what are @tsteur's thoughts on this.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 4, 2021

@sgiehl @tsteur luckily our ajaxHelper.js collects all ajax requests into an array and we already have an abort method for it:

matomo/plugins/Morpheus/javascripts/ajaxHelper.js

Lines 44 to 58 in 5e2c5dc

/**
* Extend with abort function to abort all queued requests
*
* @return {void}
*/
globalAjaxQueue.abort = function () {
// abort all queued requests
for (var i = this.length; i--;) {
this[i] && this[i].abort && this[i].abort(); // abort if possible
}
// remove all elements from array
this.splice(0, this.length);
this.active = 0;
};

So I added one more line to call this method on every reporting page load. I tested it locally and it did the job.

@Findus23
Copy link
Member

Findus23 commented Mar 4, 2021

This might also be related to #10578

@flamisz
Copy link
Contributor Author

flamisz commented Mar 4, 2021 •
edited

This might also be related to #10578

@Findus23 yes, there is a chance it solves that as well. I try to reproduce the other issue without the modifications and then with this PR.

Update: I see now, the other issue is different. In this PR we just abort the ajax requests when we change reporting page. The other issue happens with a full page reload: the new page starts loading but the browser still shows the original page, which has an error now, because the request are not loading anymore.
One solution for that one could be to delay that notification message a little bit, that time should be enough to the browser to load the new page.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 5, 2021

I updated the PR, because I find out where is that error on dashboard coming from #10578.
I added 2sec timeout for showing that error message. In this way, if the user stays on the page and an error occurs, will see it anyway, but if we clicking away from the page, the browser will have time to start loading the other page, before the error message shows up.

@diosmosis
Copy link
Member

I haven't tested this yet, but for the second issue, would it also work to cancel ajax requests in the window's unload or beforeunload events? If that would work, the solution wouldn't depend on a timeout which might avoid edge cases where it takes a long time for the next page to start loading.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 7, 2021

Hi @diosmosis, I modified the PR. Instead of using a timeout, I'm checking if the xhrStatus is aborted. If it's aborted, the error is not shown.

@flamisz flamisz requested a review from sgiehl March 9, 2021 03:49
@sgiehl sgiehl dismissed their stale review March 9, 2021 08:43

outdated

sgiehl
sgiehl approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Played around with failing requests locally, but it seems the error message now is only shown if the error really affects the current page. Guess this would be good to merge now 👍

@flamisz flamisz removed the Needs Review PRs that need a code review label Mar 9, 2021
@diosmosis diosmosis merged commit ea45527 into 4.x-dev Mar 14, 2021
@diosmosis diosmosis deleted the 17104-clear-error-message-on-page-change branch March 14, 2021 20:50
@mattab mattab modified the milestones: 4.7.0, 4.5.0 Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees

@flamisz flamisz

Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.5.0
5 participants
@flamisz @Findus23 @diosmosis @sgiehl @mattab