boxwise / boxtribute

The code base for Boxtribute 2.0, a humanitarian aid web application making it easy to source, store and distribute goods to people in need in a fair and dignified way
https://www.boxtribute.org/
Apache License 2.0
33 stars 11 forks source link

Change return type of QrCode query to better indicate errors #1512

Closed pylipp closed 4 days ago

pylipp commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 :sweat_smile: 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 commented 2 months ago

@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!

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/boxwise/boxtribute/pull/1512/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boxwise) | Coverage Δ | | |---|---|---| | [backend](https://app.codecov.io/gh/boxwise/boxtribute/pull/1512/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boxwise) | `99.12% <ø> (+<0.01%)` | :arrow_up: | | [frontend](https://app.codecov.io/gh/boxwise/boxtribute/pull/1512/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boxwise) | `77.96% <ø> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boxwise#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pylipp commented 4 days ago

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

fhenrich33 commented 4 days ago

@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 commented 4 days ago

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