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

improving UX when a http exception is encountered #11490

Merged
merged 48 commits into from
Aug 12, 2024

Conversation

RehanChalana
Copy link
Contributor

@RehanChalana RehanChalana commented Jul 13, 2024

Closes #11223

  1. Created SimpleHttpResponse class to simplify HTTP Response handling
  2. Refactored FetcherClientException to use SimpleHttpResponse, allowing for more detailed error messages and debugging
  3. replaced API keys with [REDACTED] in the error messages
  4. Created SimpleHttpResponse instance from HttpURLConnection and included it in FetcherClientException
  5. cached and displayed Fetcher Exception in the WebSearchPaneViewModel class with a dialogService.showErrorDialogAndWait() (including the response body of the SimpleHttpResponse)

I am aware the dialog box content and the way i am caching error needs improvement and I am seeking feedback on that or any changes that do not meet the quality of JabRef.

Can you please describe what should be the content of the (title and body) of the dialog box with this new approach of using SimpleHttpResponse object. and if the stack trace should be included in the dialog box as well

I am not sure how we can include (or we should not)both the http response and the redacted URL in the dialog box. Because they come from two different exceptions (parent and grandparent exceptions).

updated screenshot

image

Thank you so much for all your time and guidance

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@@ -37,7 +37,7 @@ See also: [https://help.github.com/articles/syncing-a-fork/](https://help.github
(As Administrator - one time)

1. Install [chocolatey](https://chocolatey.org)
2. `choco install git.install -y --params "/GitAndUnixToolsOnPath /WindowsTerminal"`
2. `choco install git.install -y --params "/GitAndUnixToolsOnPath /WindowsTerminal`
Copy link
Member

Choose a reason for hiding this comment

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

are you sure about that? That does not seem right...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry i am not aware when i changed the file , must have been a mistake

Copy link
Member

@calixtus calixtus Jul 14, 2024

Choose a reason for hiding this comment

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

no need to be sorry, mistakes happen and we learn. as a general rule, always check your changes before committing them. its easy if you use "git gui" to commit instead of the command line or the integrates tools in the ide.

src/main/java/org/jabref/http/dto/SimpleHttpResponse.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/http/dto/SimpleHttpResponse.java Outdated Show resolved Hide resolved
@calixtus
Copy link
Member

calixtus commented Jul 13, 2024

just some quick notes after skimming your changes. thank you for your interest in jabref!

koppor
koppor previously requested changes Jul 15, 2024
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Good initial version. I see some improvement points.

The main point is that the handling also needs to be added to org.jabref.logic.importer.EntryBasedParserFetcher#performSearch.

1. updated API_KEY_PATTERN
   a. now it matches with "key also"
   b. it only hides the api_key and does not remove everything after it

 2. moved "[REDACTED]" string to a class constant REDACTED_STRING
@RehanChalana
Copy link
Contributor Author

Good initial version. I see some improvement points.

The main point is that the handling also needs to be added to org.jabref.logic.importer.EntryBasedParserFetcher#performSearch.

How can i reproduce the error at EntryBasedParserFetcher ?

also i think we should check and remove the api_keys from org/jabref/logic/importer/PagedSearchBasedParserFetcher#getBibEntries as well

@RehanChalana
Copy link
Contributor Author

One issue i encountered in some fetchers

IOException is thrown at org/jabref/logic/net/URLDownload.java at int status = lConnection.getResponseCode();

Hence the resulting error is not an instance of FetcherClientException and hence api key is not removed from the error message

image

@koppor
Copy link
Member

koppor commented Jul 22, 2024

One issue i encountered in some fetchers

Can you try to work on it, too?

Maybe wrap it in Fetcher exception?

(Need to check the source for myself. Where is URL Download used etc. Maybe, one could introduce FetcherIoException subclassing IOexception and having the http response contained. Thus, one can add another catch where appropriate)

@RehanChalana
Copy link
Contributor Author

RehanChalana commented Jul 23, 2024

Can you try to work on it, too?

Yes of course

I will work on it and push the changes

Siedlerchr
Siedlerchr previously approved these changes Jul 24, 2024
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 24, 2024
@RehanChalana
Copy link
Contributor Author

RehanChalana commented Jul 28, 2024

One issue i encountered in some fetchers

Can you try to work on it, too?

Maybe wrap it in Fetcher exception?

(Need to check the source for myself. Where is URL Download used etc. Maybe, one could introduce FetcherIoException subclassing IOexception and having the http response contained. Thus, one can add another catch where appropriate)

I am not sure how to proceed further in this issue. The source of error seems sun/net/www/protocol/http/HttpURLConnection.java where instead of returning the status code , it is throwing IOException

image

Some guidance will be really helpful.

Also i would like to inform the way i am producing these errors is going to File-->preferences-->WebSearch and choosing Custom API keys for fetchers (i leave the value blank)

can this approach be causing some issues?

@koppor
Copy link
Member

koppor commented Aug 2, 2024

I really need to think deeper of this. Currently, I think: Can this IOException be catched early and transformed to src/main/java/org/jabref/http/dto/SimpleHttpResponse.java?

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 12, 2024
@koppor koppor marked this pull request as ready for review August 12, 2024 00:06
@koppor koppor enabled auto-merge August 12, 2024 18:11
Siedlerchr
Siedlerchr previously approved these changes Aug 12, 2024
@koppor koppor added this pull request to the merge queue Aug 12, 2024
Merged via the queue into JabRef:main with commit 9b3f768 Aug 12, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http exception should come with text
4 participants