Community
Participate
Working Groups
Created attachment 205450 [details] screenshot of funky dialog running latest code. from the workspace root, choose Link Folder, type "foo" and "foo" (This won't work). The error is reported in a funky looking dojo dialog even though our client code is trying to report the error in the status area of orion. Somehow the error is getting swallowed while deep in the bowels of the error callback installed by the service registry.
Here's some more info that may help in tracking this down. When using the progress service, some errors are being reported by the "funny dialog" and some are completely being swallowed. Wondering what the difference in the error reporting is for those cases. For example, when using "link folder" as described in comment 0, in the current build, the error is reported with the funny dialog. When the progress service is added, the dialog still comes up. But an error such as "duplicate folder" when creating a folder that already exists, is currently being reported successfully by the status service, but if you add progress handling, the error gets swallowed by the status service. Something is different about those two errors that allows one to bring up the dialog, and the other to get swallowed. The desired behavior is that whether you use the status service to report the error, or use the progress service to track the entire task, any resulting errors would be reported in our client code, not the funny dialog.
(In reply to comment #1) > When using the progress service, some errors are being reported by the "funny > dialog" and some are completely being swallowed. Wondering what the difference > in the error reporting is for those cases. I think this is from the fail/retry logic that happens in fileClient. It forwards certain error codes to auth.js, and auth puts up the funny dialog in the case of error 403 (Link Folder fail). Other error types (like 412, duplicate folder) don't trigger the dialog. I didn't debug the codepath, but it looks like the 403 gets swallowed in there too -- the promise's error handler is never called. > The desired behavior is that whether you use the status service to report the > error, or use the progress service to track the entire task, any resulting > errors would be reported in our client code, not the funny dialog. Can we remove "forbiddenAccessDlg" from the auth code entirely? I think it was a stopgap solution for when we had literally no other error handling, and it's unnecessary now. Anyone else know?
The 403 is getting swallowed in auth.js -- to be honest not sure this is at all right and even if the fileClient should be sending 403 errors to the authhandler. With that disabled I see no error handler in the createProject call on line 496 if fileCommands.
(In reply to comment #2) > Can we remove "forbiddenAccessDlg" from the auth code entirely? I think it was > a stopgap solution for when we had literally no other error handling, and it's > unnecessary now. Anyone else know? (In reply to comment #3) > The 403 is getting swallowed in auth.js -- to be honest not sure this is at all > right and even if the fileClient should be sending 403 errors to the > authhandler. Looks like we need to get rid of this funny dialog and just ensure that the proper error gets reported back so that any progress handling or status message code can report it in the expected place. The end result should be that "Link Folder" on orion.eclipse.org reports the error in the status area. > > With that disabled I see no error handler in the createProject call on line 496 > if fileCommands. The rest of the "swallowing" problem was an error in my assumptions about the progress handler error reporting. See bug 361006 if you care. Bottom line: not urgent for RC3 but we should clean this up.
(In reply to comment #4) I pushed a topic branch [1] with a fix that removes the funny dialog and uses the status service for reporting progress & errors on all the stuff in fileCommands. But then I read bug 361006, and realized this overlaps with the changes you're making there... [1] http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?h=bug361293_status&id=73d61da894b16f8eae83124833a7349c23d808f5
(In reply to comment #5) > (In reply to comment #4) > > I pushed a topic branch [1] with a fix that removes the funny dialog and uses > the status service for reporting progress & errors on all the stuff in > fileCommands. But then I read bug 361006, and realized this overlaps with the > changes you're making there... > > [1] > http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?h=bug361293_status&id=73d61da894b16f8eae83124833a7349c23d808f5 I like your solution, at least caching the error handler makes the code less complicated. And if we still decide that we want to have the progress handler do error calling for free, it's a small change to do that. Simon, do you like this for RC3? Basically it gets rid of the funny dialog and adds error handling where we didn't have it (new project)
Correction, the branch link should be: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/log/?h=bug361293_status (I hosed the commit from Comment 5, sorry)
I'm fine with this but get Mark to review and get the best fix in.
+1. Not only does this fix get rid of the dialog, but it adds proper error reporting to cases where we were doing nothing before. Tested this with: - new folder failure - new file failure - copy failure - move failure - link failure Also tested the case where you log out of the navigator, and then try the operation and it works as it should. You get the login popup, login, then back to the page and operation completes. Nice fix.
Pushed to master: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=884d09d0f1395772cab18ad9074f76dfddded5f1
Mark, this fix removed handling of 403 in any service other than fileService. Search the client code for "403", you'll see that there are still services rely of 403 handling on auth.js: * sites * users * git To reproduce go to user profile and change hash for a different user id. 403 is thrown but you get no error on UI. The leftovers still use auth.js, so maybe you could do common 403 error handling there?
+1 to just fixing the problems for FileService and leaving/rolling back the rest of the 403 handling as is in auth.js.
(In reply to comment #11) > Mark, > this fix removed handling of 403 in any service other than fileService. > Search the client code for "403", you'll see that there are still services rely > of 403 handling on auth.js: > * sites > * users > * git > To reproduce go to user profile and change hash for a different user id. 403 is > thrown but you get no error on UI. > > The leftovers still use auth.js, so maybe you could do common 403 error > handling there? Yikes, thanks for pointing this out. It would be better for the consumers of these services to handle errors, but since I can't guarantee that they do, I'll go the safe route and revert auth.js. Aside: I'm not even sure how to generate 403s for some of these cases -- I think we copied fileClient's 401/403 treatment too eagerly. I'm sure that's what happened in the sites case, at least.
Got Simon to review, pushed http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=c68492ac5e74ce73c62b7ce60773e5ebf8296435
(In reply to comment #11) > To reproduce go to user profile and change hash for a different user id. 403 is > thrown but you get no error on UI. Just a note -- this case is independently broken. the forbidden access dialog expects a JSON error, but we're getting an HTML string back from the server here.
