Bugzilla – Full Text Bug Listing
| Summary: | git-status fetch should say "Authentication required", not "Problem while performing the action" | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Adrian Aichner <adrian.aichner> | ||||||
| Component: | Git | Assignee: | Adrian Aichner <adrian.aichner> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | maciej.bendkowski | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | 5.0 RC2 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Adrian Aichner
Created attachment 238139 [details]
git-status Fetch in expired session: Problem while performing the action
How about improving this feedback to the user? (In reply to Adrian Aichner from comment #2) > How about improving this feedback to the user? Obviously, error messages like "Problem while performing the action" aren't helping much. Whenever possible, error statuses should describe the nature of the underlying problem. In your specific case, it's way to general. As pointed before, a more descriptive message, e. g. "Authentication required for: Orion Git Service. [Login] and re-try the request" is required. Regardless, this fix seems to be quite trivial. Adrian, would you care to take this bug? (In reply to Maciej Bendkowski from comment #3) > (In reply to Adrian Aichner from comment #2) > > How about improving this feedback to the user? > > Obviously, error messages like "Problem while performing the action" aren't > helping much. Whenever possible, error statuses should describe the nature > of the underlying problem. In your specific case, it's way to general. As > pointed before, a more descriptive message, e. g. "Authentication required > for: Orion Git Service. [Login] and re-try the request" is required. > Regardless, this fix seems to be quite trivial. Adrian, would you care to > take this bug? OK, please assign it to me. These seem to be catch-all messages in exception handlers. While git takes the lion share, there is one such instance in the Cloud Foundry client as well. (In reply to Adrian Aichner from comment #4) > While git takes the lion share, there is one such instance in the Cloud > Foundry client as well. Probably git and CloudFoundry are not the only places where incorrect error handling is done. Assinging to you, Adrian. Looks like eclipse.GitService</GitService.prototype._handleGitServiceResponseError@https://my.own.server:8443/orion/git/gitClient.js:971 is the central place to report a better error message. (In reply to Adrian Aichner from comment #6) > Looks like > > eclipse.GitService</GitService.prototype. > _handleGitServiceResponseError@https://my.own.server:8443/orion/git/ > gitClient.js:971 > > is the central place to report a better error message. GitClient only purpose is to serve as an API for XHR calls to the server. _handleGitServiceResponseError does what would be expected, i. e. passes raw error responses to the caller. In case of git services it would be gitCommands, or further git explorers. Error handling should be done in either gitCommands, or the appropriate explorer. This for the guidance, Maciej.
This looks like a better place then, in gitCommands.js:
exports.createFileCommands = function(serviceRegistry, commandService, explorer, toolbarId) {
function displayErrorOnStatus(error) {
The error has status 401, as expected.
20:41:22.844 console.log(new Error)
20:41:22.851 undefined
20:41:22.853 Error:
Stack trace:
@debugger eval code:1
displayErrorOnStatus@https://my.own.server:8443/orion/git/gitCommands.js:424
notify@https://my.own.server:8443/orion/Deferred.js:110
run@https://my.own.server:8443/orion/Deferred.js:29
20:43:06.940 JSON.stringify(error, function(key, value) { if (!key || value !== error) return value; }, 2)
20:43:06.947 "{
"args": {
"headers": {
"Orion-Version": "1",
"Content-Type": "application/json; charset=UTF-8",
"X-Requested-With": "XMLHttpRequest"
},
"timeout": 15000,
"handleAs": "json"
},
"response": "{\"SignInKey\":\"FORMOpenIdUser\",\"SignInLocation\":\"/mixloginstatic/LoginWindow.html\",\"label\":\"Orion workspace server\"}",
"responseText": "{\"SignInKey\":\"FORMOpenIdUser\",\"SignInLocation\":\"/mixloginstatic/LoginWindow.html\",\"label\":\"Orion workspace server\"}",
"status": 401,
"url": "/gitapi/remote/origin/master/file/adrian-OrionContent/org.eclipse.orion.server/",
"xhr": {}
}"
So my question now is how to report this error very much like
handleServiceError: function(plugin, error) {
if (error && error.status === 401) {
var headers = plugin.getHeaders();
var name = plugin.getName() || plugin.getLocation();
var span = document.createElement("span");
span.appendChild(document.createTextNode("Authentication required for: " + name + "."));
does in org.eclipse.orion.client.core / web / orion / pluginregistry.js
without duplicating that code.
I guess there should be a common way to report/handle general authentication problems.
The thing is
org.eclipse.orion.client.core / web / orion / pluginregistry.js
which provides the nice [Login] link in the notification does not use i18n
Should it also use i18n facilities?
gitCommands.js does use i18n, so I could add a message there.
But I would have to add an i18n version of
handleServiceError: function(plugin, error) {
from pluginregistry.js to gitCommands.js.
Is there a way to share the code of handleServiceError between these two modules?
(In reply to Adrian Aichner from comment #9) > The thing is > org.eclipse.orion.client.core / web / orion / pluginregistry.js > which provides the nice [Login] link in the notification does not use i18n > > Should it also use i18n facilities? A general answer is yes, wherever possible, we should use il18n. > But I would have to add an i18n version of > handleServiceError: function(plugin, error) { > from pluginregistry.js to gitCommands.js. > > Is there a way to share the code of handleServiceError between these two > modules? You shouldn't mix two possible authentication issues: 1) Plugins may require external authentication, e. g. a Google drive filesystem requiring access to your drive. This is an external authentication issue. 2) The server crashes while you're running a client command which needs to be authenticated in Orion. This is an internal authentication issue. You can simulate it by navigating to git-repository, stopping Orion and clicking on any git command. Here, you're focused on the internal authentication issues. Please follow the error control-flow in the second case for gitCommands. As an example, look at handleSshAuthenticationError or handleKnownHostsError. We should be aiming for something similar. There's no problem with il18n since you can use the provided messages map. All we really need are better error messages. (In reply to Maciej Bendkowski from comment #10) > (In reply to Adrian Aichner from comment #9) > > The thing is > > org.eclipse.orion.client.core / web / orion / pluginregistry.js > > which provides the nice [Login] link in the notification does not use i18n Thanks Maciej this answers all my questions. I have an initial attempt to improve the error message at https://github.com/anaran/orion.client/compare/Bug423492?expand=1#diff-ae40690ffdedac9459e336a342d0a34bL425 I was careful enough to keep the tab indentation, but forgot to uncheck Trim Trailing Whitespace on Save So, this first commit has a lot of trailing whitespace diffs because the file happened to have them. If the general direction this patch is going is OK, then I would upgrade the other two instances of function displayErrorOnStatus in that file as well. See the attachment for how the notification looks now. What method of review would you prefer? Thanks, Adrian > > > > Should it also use i18n facilities? > > A general answer is yes, wherever possible, we should use il18n. > > > But I would have to add an i18n version of > > handleServiceError: function(plugin, error) { > > from pluginregistry.js to gitCommands.js. > > > > Is there a way to share the code of handleServiceError between these two > > modules? > > You shouldn't mix two possible authentication issues: > 1) Plugins may require external authentication, e. g. a Google drive > filesystem requiring access to your drive. This is an external > authentication issue. > > 2) The server crashes while you're running a client command which needs to > be authenticated in Orion. This is an internal authentication issue. You can > simulate it by navigating to git-repository, stopping Orion and clicking on > any git command. > > Here, you're focused on the internal authentication issues. Please follow > the error control-flow in the second case for gitCommands. As an example, > look at handleSshAuthenticationError or handleKnownHostsError. We should be > aiming for something similar. There's no problem with il18n since you can > use the provided messages map. All we really need are better error messages. Created attachment 239667 [details]
Proposed notification on authentication failure
(In reply to Adrian Aichner from comment #11) > What method of review would you prefer? We could go for Gerrit review (see, e. g. http://wiki.eclipse.org/Orion/How_Tos/Using_Gerrit_in_Orion). Please push a proposal with minimal whitespace noise, so it's easier to review. (In reply to Maciej Bendkowski from comment #13) > (In reply to Adrian Aichner from comment #11) > > What method of review would you prefer? > > We could go for Gerrit review (see, e. g. > http://wiki.eclipse.org/Orion/How_Tos/Using_Gerrit_in_Orion). Please push a > proposal with minimal whitespace noise, so it's easier to review. While I rework the whitespace issues can you take a look at the github link I sent. That code section is the essential change. Does that and the screenshot like good? Thanks, Adrian (In reply to Adrian Aichner from comment #14) > While I rework the whitespace issues can you take a look at the github link > I sent. That code section is the essential change. > Does that and the screenshot like good? > > Thanks, Adrian I think it's a good direction, but as mentioned before, I'd like to take a look at the whole change - we should make sure, all error messages are descriptive. Therefore explorers error handling has to be corrected if required. One additional comment: please consider timeout error messages in your fix. In such cases we should have more descriptive statuses. (In reply to Maciej Bendkowski from comment #13) > (In reply to Adrian Aichner from comment #11) > > What method of review would you prefer? > > We could go for Gerrit review (see, e. g. > http://wiki.eclipse.org/Orion/How_Tos/Using_Gerrit_in_Orion). Please push a > proposal with minimal whitespace noise, so it's easier to review. I followed the instructions to set up gerrit for the client repo and was able to pull from the gerrit remote. How do I now request a review of my changes of my newly created Bug423492 branch (eliminates the many whitespace diffs), which are also available at https://github.com/anaran/orion.client/compare/Bug423492?expand=1 Thanks! Adrian (In reply to Adrian Aichner from comment #16) > How do I now request a review of my changes of my newly created Bug423492 > branch (eliminates the many whitespace diffs) It's as simple as pushing a commit to a Gerrit remote. Paste the review link in the bug. For more info on how to use Gerrit, you may refer to: http://wiki.eclipse.org/Orion/How_Tos/Using_Gerrit_in_Orion Unfortunately, we do not have a review tutorial using Gerrit on our official http://wiki.eclipse.org/Orion/Contributing_Code wiki page. We should open a separate bug for this. (In reply to Maciej Bendkowski from comment #17) Pardon me, duplicated link. In order to get familiar with Gerrit, please refer to: http://wiki.eclipse.org/Gerrit. (In reply to Maciej Bendkowski from comment #18) > (In reply to Maciej Bendkowski from comment #17) > > Pardon me, duplicated link. In order to get familiar with Gerrit, please > refer to: http://wiki.eclipse.org/Gerrit. Thanks! My actual stopper was outdated information in https://wiki.eclipse.org/Orion/Contributing_Code#Commit_your_changes Leading dashes in --Signed-off-by: ... caused gerrit to reject my upload saying remote: error: The author has not "signed-off" on the contribution. I amended my commit removing the dashes and gerrit was happy. I have also updated above wiki page. Please see https://git.eclipse.org/r/#/c/21709/ for my proposed change. Hi Maciej, just in case this got lot in my chatter, could you please let me know if my gerrit submission looks formally correct? Please see https://git.eclipse.org/r/#/c/21709/ for my proposed change. I just want to avoid a dead lock in case you are expecting something else. Thanks! Adrian Merged to master: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b67182484472eadeaa69c210b1bf511f9a0fe04c Thanks, Adrian! |