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

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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pylipp
Copy link
Contributor

@pylipp pylipp commented Sep 9, 2024

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 pylipp force-pushed the change-qrcode-query-interface branch 2 times, most recently from 564cafe to f7b634e Compare September 9, 2024 14:50
The unions may contain error types with detailed info
@pylipp pylipp force-pushed the change-qrcode-query-interface branch from f7b634e to c1c8ecc Compare September 9, 2024 14:51
@pylipp pylipp force-pushed the change-qrcode-query-interface branch from c1c8ecc to 7ee08fb Compare September 9, 2024 15:01
@pylipp
Copy link
Contributor Author

pylipp commented Sep 9, 2024

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

@pylipp pylipp force-pushed the change-qrcode-query-interface branch from a7170c1 to 93beadb Compare September 9, 2024 15:52
@pylipp pylipp changed the title Change QrCode query return type to better indicate errors Change return type of QrCode query to better indicate errors Sep 9, 2024
@fhenrich33
Copy link
Contributor

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

Copy link
Member

@HaGuesto HaGuesto left a comment

Choose a reason for hiding this comment

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

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
    }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants