Change return type of QrCode query to better indicate errors

[pylipp]

The unions may contain error types with detailed info. I hope the following table clarifies how to deal with the response of the query https://github.com/boxwise/boxtribute/blob/change-qrcode-query-interface/front/src/queries/queries.ts#L38-L66

Union fragment	Only set if ...	Related current response	Current handling in FE
1. QrCodeResult.InsufficientPermissionError	...user misses the qr:read permission (e.g. a label-creation user)	error code FORBIDDEN	message "You don't have permission to access this box!"
2. QrCodeResult.ResourceDoesNotExistError	...the QR code for the requested hash does not exist in the database	error code BAD_USER_INPUT	message "No box found for this QR code!"
3. QrCodeResult.QrCode	...the errors 1.&2. don't occur	QR code data	success; redirect accordingly
3a. QrCodeResult.BoxResult.InsufficientPermissionError	...if 3. happens AND user misses the stock:read permission	like in 1.	like in 1.
3b. QrCodeResult.BoxResult.UnauthorizedForBaseError	...if 3. happens AND user has stock:read permission BUT NOT for the base that the requested box is in	like in 1.	like in 1.
3c. QrCodeResult.BoxResult.Box	...if the errors 3a. and 3b. don't occur	box data (null if QrCode is not associated with any box)	success

Note how currently the cases 1, 3a, 3b cannot be distinguished.

Also with the new interface design now in case of 3b information about the base/org of the box can be displayed.

[pylipp]

When scanning the QR code of a box registered at another organisation, instead of showing an error about authz failure, show information about which organisation and base the box actually belongs to (i.e. copy behavior of dropapp).

[pylipp]

@fhenrich33 I consider this completed from a BE perspective. Please check the first post about the different return types of the qrCode query and the QrCode.box field. Overall, the logic in useQrResolver needs a major rework now 😅 let me know how it goes for you. I could already adjust queries.ts though.
Since the changes in the GraphQL API break FE, I suggest you continue to work on it on the same branch, what do you think?

[fhenrich33]

@fhenrich33 I consider this completed from a BE perspective. Please check the first post about the different return types of the qrCode query and the QrCode.box field. Overall, the logic in useQrResolver needs a major rework now 😅 let me know how it goes for you. I could already adjust queries.ts though. Since the changes in the GraphQL API break FE, I suggest you continue to work on it on the same branch, what do you think?

Makes sense, will do!

[HaGuesto]

Thanks @pylipp! Great PR until now.

code review

It passes, but some FYI for @fhenrich33 :

• Philipp changed the input variable name for qrCode and qrExists query. We should check the places where we execute the query.

functional review

passes as well. This is the query I used to test:

{
  qrExists(code: "093f65e080a295f8076b1c5722a46aa2")
  qrCodeFail: qrCode(code: "093f65e080a295f8076b1c5722a46aa2") {
    ... on QrCode {
      id
      box {
        ... on Box {
          labelIdentifier
        }
        ... on InsufficientPermissionError {
          name
        }
        ... on UnauthorizedForBaseError {
          name
          organisationName
        }
      }
    }
    ... on InsufficientPermissionError {
      name
    }
    ... on InsufficientPermissionError {
      name
    }
  }
  qrCodeValid: qrCode (code: "9d7f34fbea04de63b0acdfb874f3ef59") {
    ... on QrCode {
      id
      box {
        ... on Box {
          labelIdentifier
        }
        ... on InsufficientPermissionError {
          name
        }
        ... on UnauthorizedForBaseError {
          name
          organisationName
        }
      }
    }
    ... on InsufficientPermissionError {
      name
    }
    ... on InsufficientPermissionError {
      name
    }
  }
}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.33%. Comparing base (5caa2d9) to head (2388dd8).
Report is 24 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1512      +/-   ##
==========================================
- Coverage   84.35%   84.33%   -0.02%     
==========================================
  Files         236      236              
  Lines       14775    14823      +48     
  Branches     1579     1582       +3     
==========================================
+ Hits        12463    12501      +38     
- Misses       2273     2283      +10     
  Partials       39       39              

Flag	Coverage Δ
backend	99.12% <ø> (+<0.01%)	⬆️
frontend	77.96% <ø> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pylipp]

@fhenrich33 I'll merge master into this branch and try to resolve the conflict in the mutations.graphql file.

[pylipp]

I think this should be NO_BOXTRIBUTE_QR and also show an error toast?

[fhenrich33]

This is handled at /front/src/components/QrReader/QrReaderContainer.tsx

case IQrResolverResultKind.NOT_ASSIGNED_TO_BOX: {
  if (!multiScan) {
    onSuccess();
    navigate(`/bases/${baseId}/boxes/create/${qrResolvedValue?.qrHash}`);
  } else {
    triggerError({
      message: "No box associated to this QR code!",
    });
    setIsProcessingQrCodeDelayed(false);
  }
  break;
}

[pylipp]

yes it works 👍 But I end up on an empty page http://localhost:3000/bases/1/boxes/create/13f12820c8010f2f7349962930e6bf4 (I used the last one of the invalid QR codes from the repo readme).
I'd expect to stay on /qrread

[fhenrich33]

Huh, this should've been handled at front/src/hooks/useQrResolver.ts

const resolveQrCode = useCallback(
  async (qrCodeUrl: string, fetchPolicy: FetchPolicy): Promise<IQrResolvedValue> => {
    setLoading(true);
    const extractedQrHashFromUrl = extractQrCodeFromUrl(qrCodeUrl);

    if (!extractedQrHashFromUrl) {
      triggerError({
        message: "This is not a Boxtribute QR code!",
      });

      setLoading(false);
      return { kind: IQrResolverResultKind.NOT_BOXTRIBUTE_QR } as IQrResolvedValue;
    }

    const qrResolvedValue: IQrResolvedValue = await resolveQrHash(
      extractedQrHashFromUrl,
      fetchPolicy,
    );

    setLoading(false);
    return qrResolvedValue;
  },
  [resolveQrHash, triggerError],
);

Cheking this out.

[fhenrich33]

@pylipp does this happen in the live version?

[fhenrich33]

@pylipp By the way the tests that are failing doesn't reflect what actually happens in the real app, I didn't figure out a way to fix them yet. cc @HaGuesto

The failing tests:

         3.4.3.5 QrReaderMultiBox.test.tsx
            AssertionError: expected "spy" to be called with arguments: [ ObjectContaining{…} ]

            Received:

            1st spy call:

            Array [
            -   ObjectContaining {
            -     "message": StringMatching /have permission to access this box/i,
            +   Object {
            +     "message": "QR code lookup failed. Please wait a bit and try again.",
                },
            ]
        3.4.8.7 ResolveHash.test.tsx
            TestingLibraryElementError: Unable to find an element by: [data-testid="ReturnScannedQr"]
        3.4.2.2 QrReaderOverlay.test.tsx
            TestingLibraryElementError: Unable to find role="heading" and name "/bases/1/boxes/123"
        3.4.2.5b QrReaderOverlay.test.tsx 
            AssertionError: expected "spy" to be called with arguments: [ ObjectContaining{…} ]

            Received:

            1st spy call:

            Array [
            -   ObjectContaining {
            -     "message": StringMatching /No box found for this QR code/i,
            +   Object {
            +     "message": "QR code lookup failed. Please wait a bit and try again.",
                },
            ]

[HaGuesto]

@pylipp this PR is ready to be merge. Pls do so when you think it is the right time.