Bugzilla – Full Text Bug Listing
| Summary: | improve error message in case of missing Bugzilla_login cookie | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Daniel <daniel.brugger> | ||||||||||||||||||||
| Component: | Mylyn | Assignee: | Frank Becker <eclipse> | ||||||||||||||||||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||||||||||||||||||
| Severity: | minor | ||||||||||||||||||||||
| Priority: | P3 | CC: | steffen.pingel | ||||||||||||||||||||
| Version: | unspecified | ||||||||||||||||||||||
| Target Milestone: | 3.11 | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||
| Bug Blocks: | 268207, 359210 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Daniel
Created attachment 129769 [details]
patch
Created attachment 129770 [details]
mylyn/context/zip
Since if a token isn't present this always represents a failure to log in (not an exceptional case) we should test that the web page returned says we're logged in but we don't get a token. I'd like to keep this simple though (i.e. maybe a global token received field combined with a test in the parse error method) thoughts? Created attachment 136711 [details] next version (In reply to comment #3) > Since if a token isn't present this always represents a failure to log in (not > an exceptional case) we should test that the web page returned says we're > logged in but we don't get a token. I'd like to keep this simple though (i.e. > maybe a global token received field combined with a test in the parse error > method) thoughts? Rob, what do you think about this way for fixing the problem. Created attachment 136712 [details]
mylyn/context/zip
(In reply to comment #4) > Rob, what do you think about this way for fixing the problem. Frank this looks like the right idea. What if instead of looking for the 'bugzilla main page' we test for absense of the GoAheadAndLogIn=1 text within the page? This way we might have less failures for customized bugzillas out of the box? This might need to move out of the error parsing though. Thoughts? "You have been logged in but the cookie was not found. Please retry operation." could read: "An authentication cookie was not issued by repository. Check repository credentials and retry." Frank, can you update the patch based on Rob's comment? create http://review.mylyn.org/352 Created attachment 211997 [details]
mylyn/context/zip
This looks reasonable but I would like to keep the cookie check. We should only parse the page if the cookie is not found. Steffe, here is Patchset 4 http://review.mylyn.org/#change,352,patchset=4 That looks good. We would just need a test case to verify that the parsing works as expected. (In reply to comment #12) > That looks good. We would just need a test case to verify that the parsing works > as expected. I create a new review http://review.mylyn.org/#change,357 because I recreate the way to solve this during junit creation. Created attachment 212033 [details]
mylyn/context/zip
Makes sense to do that as part of the standard parsing but this doesn't look right to me. It would seem perfectly valid that the GoAheadAndLogin token is part of the response in case of anonymous access for instance? The changes are in parseRepositoryResponse and used in parseHtmlError (TaskID is null) and parsePostResponse (use TaskID). parseRepositoryResponse use * Tag.TITLE * Tag.TD with id = error_msg and now new * Tag.A with attribute href contains GoAheadAndLogIn=1 So I did not see where we should use parseRepositoryResponse and did not use it. I am worried that the code detects an error in a case where the GoAheadAndLogin tag is legitimate. Let's apply this to master and refactor the code so we can make the error parsing more flexible. (In reply to comment #17) > I am worried that the code detects an error in a case where the GoAheadAndLogin > tag is legitimate. Let's apply this to master and refactor the code so we can > make the error parsing more flexible. Should we parse the whole response and build the result from all three cases • Tag.TITLE • Tag.TD with id = error_msg • Tag.A with attribute href contains GoAheadAndLogIn=1 So we can build an priority in which order we report the errors Thoughts? Yes, that sounds like a good idea. (In reply to comment #19) > Yes, that sounds like a good idea. I create the review http://review.mylyn.org/#change,376 with the new implementation. Created attachment 212832 [details]
mylyn/context/zip
Review is now here. https://git.eclipse.org/r/5958 create new patch set Created attachment 230752 [details]
mylyn/context/zip
new patch set 4 created. Created attachment 231267 [details]
mylyn/context/zip
move the target milestone to 3.11. Need to recreate the patch after we have merged the review 414360: add support for the new REST API (Bugzilla 5.0) create AbstractBugzillaClient (refactor) (https://git.eclipse.org/r/#/c/17206/) Should we move this to the backlog? Maybe we could just close this and worry about about improved error handling as part of the REST connector design? (In reply to Steffen Pingel from comment #29) > Maybe we could just close this and worry about about improved error handling > as part of the REST connector design? We will address this in the REST implementation. |