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

Issuexxxx improve http error handling #1033

Closed
wants to merge 6 commits into from

Conversation

kouralex
Copy link
Contributor

@kouralex kouralex commented Jul 16, 2020

This PR fixes a bunch or problems in current code.

Fixes the following listed issues:
Fixes #693
Fixes #721
Fixes #722
Fixes #723
Fixes #1011

Addresses #759 in the sense that it could be implemented in a similar way (add a retry button/handle ajax failure)

Adds PHP fatal error checks for various easyrdf http client induced errors. E.g., doesn't crash the UI in the usual cases for a non-responding SPARQL endpoint (some may still exist).
Global search: adds a warning for every selected, non-default endpoint vocabularies.

Searches may now return 500 Internal Server Error, which on turn is utilized by Ajax queries. Please note that not every failure results in an HTTP 500 Error.
Waypoint system in searches is fixed; no more extra requests.

TODO/open questions:

  • Should global search warnings lie above the number of results?
  • Translations for various new JS and Twig template variables
  • Tests? (and for which part)
  • Is the way errors are handled here the best possible?
  • Add a retry button to other parts of UI?
  • Is the usage of HTTP 500 good as it is?
  • Sidebar could be separated for good from the content area, this PR only has started the process.
  • Also, it is possible to merge the functionality of Skosmos into a single page app by developing this a bit further.

@kouralex kouralex added this to the Next Tasks milestone Jul 16, 2020
@kouralex kouralex requested review from joelit and osma July 16, 2020 22:45
@sonarcloud
Copy link

sonarcloud bot commented Jul 16, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

if ($this->model->getConfig()->getLogCaughtExceptions()) {
error_log('Caught exception: ' . $e->getMessage());
}
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this ok?

@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #1033 into master will decrease coverage by 0.65%.
The diff coverage is 15.38%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1033      +/-   ##
============================================
- Coverage     60.32%   59.67%   -0.66%     
- Complexity     1593     1612      +19     
============================================
  Files            32       32              
  Lines          4429     4484      +55     
============================================
+ Hits           2672     2676       +4     
- Misses         1757     1808      +51     
Impacted Files Coverage Δ Complexity Δ
controller/WebController.php 14.16% <0.00%> (-0.82%) 88.00 <0.00> (+5.00) ⬇️
model/Model.php 78.37% <0.00%> (-3.15%) 109.00 <0.00> (+4.00) ⬇️
model/DataObject.php 63.26% <21.42%> (-12.35%) 27.00 <0.00> (+4.00) ⬇️
model/Vocabulary.php 82.77% <40.90%> (-4.03%) 118.00 <0.00> (+6.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37601f9...9f762ff. Read the comment docs.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

Bundling so many fixes into a single PR makes it very difficult to review. Could you please split this into smaller, more focused PRs?

@kouralex
Copy link
Contributor Author

Thank you for your valuable feedback @osma .

I just gave a quick look at this draft PR after it has been a while since I made this and it seemed like that except for the waypoint fixes (that just happened to annoy me at the time of building this PR up) and #1011-related part (which has been by the way fixed in another commit already (now) residing in the master branch) this PR truly fixes (or at least tries to fix, as it says on the PR comment, there are still things to be done) one big problem that has just been reported in multiple places - well OK, to be honest, one could separate most UI fixes into their own PR but then again - they depend on this PR as this changes the way they (that is to say, UI) expect issues are passed down (catching them). One can, of course, drop them from this PR (which may, by the way, cause other hassle as the UI is not prepared for aforementioned changes) but I personally think this is a matter of personal preference for how to test that things truly are working in the very UI without writing the unit tests first. Anyways, anything needed for a successful review is there in the PR comment - after all, this is a draft PR and some comments are appreciated and very much needed (see the list of TODOs/open questions) in order to finish this PR.

@kouralex kouralex added size-medium 2 hours to 2 days needs discussion needs unittest size-large more than 2 days enhancement and removed size-medium 2 hours to 2 days labels Feb 26, 2021
@kouralex kouralex requested a review from osma September 2, 2021 08:17
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

I gave some quick comments.
The biggest problem ATM is the conflicts that should be resolved before this PR can even be tested with the code in current master.

<div class="alert alert-warning">
{% for voc in skipped_vocabs %}
{% set vocTitle = voc.title %}
<h4>{% trans %}Warning: Skipped searching from '{{vocTitle}}' vocabulary!{% endtrans %}</h4>
Copy link
Member

Choose a reason for hiding this comment

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

We need translations for this new UI string

<h4>{% trans %}Error: Requested vocabulary not found!{% endtrans %}</h4>
{% else %}
<h4>{% trans %}Error: Search failed!{% endtrans %}</h4>
Copy link
Member

Choose a reason for hiding this comment

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

We need translations for this new UI string

{% set vocabInfo = vocab.info(request.contentLang) %}
{% if not vocabInfo %}
<div class="alert alert-danger" role="alert">
{% trans "Error: Failed to retrieve vocabulary information!" %}
Copy link
Member

Choose a reason for hiding this comment

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

We need translations for this new UI string

},
error: function(jqXHR, textStatus, errorThrown) {
$loading.detach();
var $failedSearch = $('<div class="alert alert-danger"><h4>Error: Loading for more items failed!</h4></div>');
Copy link
Member

Choose a reason for hiding this comment

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

This error message should be translated

error: function(jqXHR, textStatus, errorThrown) {
$loading.detach();
var $failedSearch = $('<div class="alert alert-danger"><h4>Error: Loading for more items failed!</h4></div>');
var $retryButton = $('<button class="btn btn-default" type="button">Retry</button>');
Copy link
Member

Choose a reason for hiding this comment

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

"Retry" should be translated as well

$vocabObjects[] = $this->model->getVocabulary($vocid);
try {
$vocabObjects[] = $this->model->getVocabulary($vocid);
} catch (Exception $e) {
Copy link
Member

Choose a reason for hiding this comment

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

An empty catch block is bad practice. What happens with the $vocabObjects variable that is used below?
At a minimum, it could be set to null here, or array() if that makes more sense.

@osma
Copy link
Member

osma commented Sep 7, 2021

It's not possible to do proper review on this PR as it mixes up so many different fixes - and it's even more difficult now that the branch has diverged from master and ended up in conflicted state. I will split this into separate branches (mainly by commits) and open PRs for each of them.

@osma
Copy link
Member

osma commented Sep 7, 2021

I've now disentangled most of the changes included in this PR into separate PRs:

I think these can be handled separately in those PRs.

What remains is the last commit (so far) in this PR 9f762ff which is itself a bundle of several fixes in a single commit, and it seems that it may rely on changes done in the previous commits, so cannot easily be separated. I think the above PRs should be merged first and then the remaining commit can be dealt with.

@osma
Copy link
Member

osma commented Sep 9, 2021

The remaining parts of this PR (well, except the message about skipped vocabularies, which was deemed too obtrusive) have been merged via PR #1197. Closing this one.

@osma osma closed this Sep 9, 2021
@joelit joelit modified the milestones: Next Tasks, No further action Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment