adobe / commerce-cif-connector

AEM Commerce connector for Magento and GraphQL
Apache License 2.0
43 stars 27 forks source link

CIF-1245 - Connector fails if Magento GraphQL response has errors. #98

Closed LSantha closed 4 years ago

LSantha commented 4 years ago

Description

The user needs some error reporting in the console if the product tree binding doesn't load products due to internal error. The node having the error is highlighted with a Granite alert.

Related Issue

CIF-1245

How Has This Been Tested?

Manually.

Screenshots (if appropriate):

Server side error reporting:

error

error2

error3

Client side error reporting error

Types of changes

Checklist:

codecov[bot] commented 4 years ago

Codecov Report

Merging #98 into master will increase coverage by 0.19%. The diff coverage is 87.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #98      +/-   ##
============================================
+ Coverage     87.45%   87.65%   +0.19%     
- Complexity      454      460       +6     
============================================
  Files            33       34       +1     
  Lines          1794     1822      +28     
  Branches        272      277       +5     
============================================
+ Hits           1569     1597      +28     
+ Misses          102       99       -3     
- Partials        123      126       +3     
Flag Coverage Δ Complexity Δ
#integration 49.85% <12.12%> (-0.85%) 250.00 <6.00> (+1.00) :arrow_down:
#karma 92.71% <ø> (ø) 0.00 <ø> (ø)
#unittests 83.90% <87.87%> (+0.27%) 444.00 <13.00> (+6.00)
Impacted Files Coverage Δ Complexity Δ
.../adobe/cq/commerce/graphql/resource/Constants.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...e/cq/commerce/graphql/resource/ResourceMapper.java 79.47% <87.50%> (+2.37%) 40.00 <10.00> (+3.00)
...be/cq/commerce/graphql/resource/ErrorResource.java 88.23% <88.23%> (ø) 3.00 <3.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 e5ee213...72c7ff0. Read the comment docs.

mhaack commented 4 years ago

Did not check the code yet :-) but only the screenshot we need to talk about the implementation. I don't think this way of display an error as a column item is the right way to do it. There are error & notification elements in the UI framework. Also, how will this look like in card or list view?

LSantha commented 4 years ago

@mhaack , I've added screenshots for list and card view too. I agree this is quite unusual, I'm actually using a resource to communicate the error to the client this is because throwing and exception from the Resource API implementation is not something that all the possible receiving components can handle properly and the result is that the console gets broken in various situations. The error message is displayed using alert available in Granite. We can still discuss it, and think about alternatives, I'm OK with that.