feat: map errors into an object #1108

ivopereira15 · 2022-04-07T16:39:37Z

PR Type

[ X] Bugfix
[ X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no API changes)
[ ] Build-related changes
[ ] CI-related changes
[ ] Documentation content changes
[ ] Application / infrastructure changes
[ ] Other:

What Is the Current Behavior?

At this moment when we have more than one error we are showing an warning on browser console. This is a problem in the quick order because when we have more than one product with error/errors we dont show feedback to the customer.
Also when we are showing the error we dont know which product we are having the error.

What Is the New Behavior?

With this change we are fixing two problems

Show error when we have more than one
Have the possibility to know which product is having an error.

We are adding a property named causes to the HttpError. This property will have the error message with more details and if it has will have the product sku.

Examples
Quick order when we are trying to add two products

Does this PR Introduce a Breaking Change?

[ ] Yes
[ X] No

Other Information

partial case not mapped yet

M-Behr · 2022-04-25T09:51:42Z

Thank you for your feedback. I see 3 variants to improve the error case behavior:

Var 1:
2 products could not be added. (--> general error)
The product quantity for these items in your cart... (--> reason summary for same errors)

Var 2:
2 products could not be added. (--> general error)
The product quantity for "Microsoft Surface Hub" in your shopping cart... (--> reason 1)
The product quantity for "Microsoft Surface Book" in your shopping cart... (--> reason 2)

Alternative: The product quantity for product with SKU "123456" in your shopping cart...
(Is this really helpful because we don't show SKUs in mini cart?)

Var 3:
If error handling is hard to overview in mini cart, we could display always shopping cart page in cases of errors or adjustments (see Wireframes).

We will check your solution and discuss if it's fits to our interaction guidelines.

M-Behr · 2022-04-25T11:50:49Z

I discussed the error behavior with @SGrueber.
We want to jump to the cart page if we get multiple product errors after adding products to the cart. There we can display the individual errors directly on the product and make it much clearer. It should be look like that:

We would continue to display error messages for a single product in the mini shopping cart. We would leave the behavior for this case as it is.

@ivopereira15 @dhhyi Is this behavior fine for you guys?

dhhyi · 2022-04-25T11:59:08Z

@M-Behr Yes, the main goal for us is that errors are mapped and available, so if that part of the PR is okay, we are good. We have customized the display in our project using toasts 😉
@SGrueber Will you take care of the behavior for the standard PWA?

cc @ivopereira15

SGrueber · 2022-04-29T09:47:25Z

Yes, I will

SGrueber · 2022-05-05T08:45:10Z

The error display in the mini basket looks like this now:

MaxKless · 2022-05-05T09:21:05Z

Hi @ivopereira15, can you let me know why you used the info basket substate to track the errors & causes? Instead of the error substate?
It would seem a bit cleaner that way but maybe that comes with some additional disadvantages. Could you provide some context?

ivopereira15 · 2022-05-05T16:02:08Z

Hi all. @MaxKless

The error substate its working only when you have a bad request. Like a Status Code: 422 Unprocessable Entity. This is a clear error message that will be handled by addItemsToBasketFail

Lets imagine in the quickorder (that was one the problems that we found) we are trying to add to cart two products. One that we will have an error and other will be success. The response is Status Code: 207 Multi-Status.
That means we are not mapping to an error. We are going to theaddItemsToBasketSuccess action and mapping to an info object.

here what we receive from the request.

What i did was put that error inside the infos object. Since we already were showing alert/info error using the info object, could be a good idea to show the error too.
With that infos object we have the info and the errors on the same place.

MaxKless · 2022-05-30T14:58:36Z

src/app/core/models/basket-info/basket-info.model.ts


 export interface BasketInfo {
  causes?: BasketFeedback[];
  code: string;
  message: string;
+  error?: ErrorFeedback[];


since this is an array, it should be called errors imo.

MaxKless · 2022-05-31T07:49:17Z

src/app/core/models/http-error/http-error.model.ts

@@ -10,4 +23,6 @@ export interface HttpError {

  /** human readable (and localized) error message */
  message?: string;
+
+  causes?: ErrorFeedback[];


Should be a documented property like the others.
I also think it would be good to rename the property - see comment on mini-basket-content.component.html

MaxKless · 2022-05-31T09:20:22Z

src/app/pages/basket/shopping-basket-error-message/shopping-basket-error-message.component.ts

+ */
+@Component({
+  selector: 'ish-shopping-basket-error-message',
+  template: '',


Having a component without a template isn't very nice. Something is created in the DOM but only used for state management, kind of like an effect.
Since I understand that this logic is component-specific and doesn't belong in an effect, I would extract the logic to the parent component (shopping-basket.component) and subscribing there or using rx state's hold method to to trigger a component state side-effect like the context facades do it.

MaxKless · 2022-05-31T10:05:56Z

src/app/pages/basket/shopping-basket-error-message/shopping-basket-error-message.component.ts

+      if (infoError) {
+        infoError?.map(error => {
+          this.messageFacade.error({
+            message: `<b>${error.message}</b> - ${error.causes?.[0].message} ${error.causes?.[0].parameters?.sku}`,


Can you provide some insight as to why only the first error is dispatched here?

MaxKless · 2022-05-31T10:09:41Z

src/app/shared/components/basket/mini-basket-content/mini-basket-content.component.html

+  <ng-container *ngIf="error.causes as productCauses">
+    <div *ngFor="let productCause of productCauses" class="text-danger">
+      <strong>{{ productCause.message }}</strong>
+      <ng-container *ngIf="productCause.causes as causes">


I think the naming could be improved here. The fact that theres two nested causesproperties in the model really makes this code incredibly hard to read and confusing. Since the top-levelcausesproperty is of typeErrorFeedback`, maybe the naming could be similar to that?
What do you think?

MaxKless · 2022-05-31T10:11:28Z

...p/pages/basket/shopping-basket-error-message/shopping-basket-error-message.component.spec.ts

+    when(checkoutFacade.basketInfoError$).thenReturn(of(undefined));
+  });
+
+  it('should be created', () => {


I think the behavior currently defined in the component is pretty straightforward to test and we should probably do so wherever it ends up.

MaxKless · 2022-05-31T11:11:35Z

src/app/core/facades/checkout.facade.ts

@@ -85,6 +85,7 @@ export class CheckoutFacade {
  basketChange$ = this.basketChangeInternal$.asObservable();
  basketError$ = this.store.pipe(select(getBasketError));
  basketInfo$ = this.store.pipe(select(getBasketInfo));
+  basketInfoError$ = this.basketInfo$.pipe(map(info => (info?.length ? info[0].error : undefined)));


Maybe this should be its own selector?

Also, maybe this is the wrong place to ask this so please let me know if that's the case but can you explain the store structure here a bit? @SGrueber

How come there's both a basket.info as well as a info substate in the basket state?

basket.info contains info of the get basket (or the result of any call that returns the whole basket ). The info sub state contains the info of the last basket operation, e.g. addProductToCart. Because after each basket operation a get basket is triggered we need both info values.

This reverts commit 46ff1dc.

Co-authored-by: Silke Grueber <s.grueber@intershop.de>

SGrueber · 2022-08-02T14:40:42Z

has been merged by #1230

dhhyi requested review from SGrueber and dhhyi April 12, 2022 07:44

dhhyi previously approved these changes Apr 12, 2022

View reviewed changes

dhhyi self-requested a review April 13, 2022 10:01

dhhyi previously approved these changes Apr 20, 2022

View reviewed changes

SGrueber self-assigned this Apr 25, 2022

SGrueber added this to the 2.3 milestone Apr 25, 2022

SGrueber requested a review from M-Behr April 25, 2022 08:03

SGrueber dismissed dhhyi’s stale review via 68a3bd8 April 25, 2022 09:32

SGrueber force-pushed the feat--map-error-messages-on-object branch from d3dbd18 to 68a3bd8 Compare April 25, 2022 09:32

shauke modified the milestones: 2.3, 2.4 May 3, 2022

SGrueber force-pushed the feat--map-error-messages-on-object branch from 20fc4a9 to 795c913 Compare May 4, 2022 08:10

M-Behr previously approved these changes May 5, 2022

View reviewed changes

shauke force-pushed the develop branch from 9a26d87 to dc92011 Compare May 5, 2022 09:49

SGrueber dismissed M-Behr’s stale review via 067ef0c May 5, 2022 16:26

SGrueber requested review from MaxKless and removed request for SGrueber May 6, 2022 13:53

shauke assigned MaxKless May 25, 2022

MaxKless modified the milestones: 2.4, 3.0 May 31, 2022

MaxKless suggested changes May 31, 2022

View reviewed changes

github-actions bot added the has conflicts label Jul 28, 2022

Ivo Pereira and others added 10 commits July 29, 2022 11:51

map errors into an object

b39ce39

remove code

7a21b19

add error interface to the error model. fix tests

8e655f8

replace causes to cause

4437128

fix cypress tests

6b5ce64

show info errors

95d0a15

Revert "replace causes to cause"

ff59b83

This reverts commit 46ff1dc.

feat: enhance display of error messages in the mini basket

ae9eed0

refactor: display error and info messages on the shopping cart

b2b8b35

fix: basket promotion handling e2e test - display of the error message

d867ffa

SGrueber force-pushed the feat--map-error-messages-on-object branch from 067ef0c to d867ffa Compare July 29, 2022 09:58

github-actions bot removed the has conflicts label Jul 29, 2022

SGrueber mentioned this pull request Jul 29, 2022

feat: display multiple basket errors #1230

Merged

SGrueber added a commit that referenced this pull request Aug 2, 2022

feat: display multiple basket errors (#1230 #1108)

399a5cb

Co-authored-by: Silke Grueber <s.grueber@intershop.de>

SGrueber added a commit that referenced this pull request Aug 2, 2022

feat: display multiple basket errors (#1230 #1108)

c0d73c7

Co-authored-by: Silke Grueber <s.grueber@intershop.de>

github-actions bot added the has conflicts label Aug 2, 2022

SGrueber closed this Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: map errors into an object #1108

feat: map errors into an object #1108

ivopereira15 commented Apr 7, 2022

M-Behr commented Apr 25, 2022

M-Behr commented Apr 25, 2022

dhhyi commented Apr 25, 2022

SGrueber commented Apr 29, 2022

SGrueber commented May 5, 2022

MaxKless commented May 5, 2022

ivopereira15 commented May 5, 2022 •

edited

Loading

MaxKless May 30, 2022

MaxKless May 31, 2022

MaxKless May 31, 2022

MaxKless May 31, 2022

MaxKless May 31, 2022

MaxKless May 31, 2022

MaxKless May 31, 2022

SGrueber Jul 29, 2022

SGrueber commented Aug 2, 2022

feat: map errors into an object #1108

feat: map errors into an object #1108

Conversation

ivopereira15 commented Apr 7, 2022

PR Type

What Is the Current Behavior?

What Is the New Behavior?

Does this PR Introduce a Breaking Change?

Other Information

M-Behr commented Apr 25, 2022

M-Behr commented Apr 25, 2022

dhhyi commented Apr 25, 2022

SGrueber commented Apr 29, 2022

SGrueber commented May 5, 2022

MaxKless commented May 5, 2022

ivopereira15 commented May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SGrueber commented Aug 2, 2022

ivopereira15 commented May 5, 2022 •

edited

Loading