Skip to content

Conversation

@jamescammarano
Copy link
Contributor

@jamescammarano jamescammarano commented May 27, 2022

Description

We have generate-joi/index.ts in the app with a comment that the function should be shared. We also have generate-joi.ts in the shared package. They both (to me) seem to do the same thing but slightly differently.

The benefit of merging them is that operators, updates, and optimizations don't need to be added in both places.

I also updated the app/jest.config.js to properly resolve @/ imports.

NOTE: Neither implementation seems to support _and or _or

Fixes #13595
Fixes #13855
Fixes #13119

Type of Change

  • Bugfix
  • New Feature
  • Refactor / codestyle updates
  • Other, please describe:

Requirements Checklist

  • New / updated tests are included
  • All tests are passing locally
  • Performed a self-review of the submitted code

If adding a new feature:

  • Documentation was added/updated

@jamescammarano jamescammarano marked this pull request as ready for review May 27, 2022 22:44
@rijkvanzanten rijkvanzanten self-assigned this May 31, 2022
@rijkvanzanten rijkvanzanten added this to the v9-next milestone Jun 8, 2022
@rijkvanzanten rijkvanzanten requested review from licitdev and removed request for licitdev and rijkvanzanten June 8, 2022 19:15
@rijkvanzanten
Copy link
Member

@licitdev could you do a review to make sure the behavior is consistent across the original uses of the util? I'm mostly worried about the difference in allowUnknown causing new issues 🤔

@licitdev
Copy link
Member

licitdev commented Jun 8, 2022

@rijkvanzanten Yes, The allowUnknown() aspect was one of the differences between the app & api util. This is not a trivial merge but a necessary one. 👍🏻

I'm worried about regressions, particularly because it's used in parsing filters in permissions. Will need some time to investigate deeper.

@licitdev licitdev marked this pull request as ready for review June 15, 2022 13:42
@rijkvanzanten rijkvanzanten self-assigned this Jun 15, 2022
@jamescammarano
Copy link
Contributor Author

What kind of tests does this need to be merged? I can whip some up today.

@licitdev
Copy link
Member

This PR also fixes #13119 from the updated app/src/utils/is-allowed.ts.

@rijkvanzanten rijkvanzanten added this to the Next Release milestone Jul 11, 2022
@rijkvanzanten rijkvanzanten merged commit d57ea95 into main Jul 11, 2022
@rijkvanzanten rijkvanzanten deleted the generate-joi-merge branch July 11, 2022 17:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

6 participants