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

CTB: Improve UI validation for enum values #13717

Merged
merged 8 commits into from Jul 18, 2022
Merged

Conversation

gu-stav
Copy link
Contributor

@gu-stav gu-stav commented Jul 7, 2022

What does it do?

Builds on top of #13503, to improve the enum values validation in the admin UI. Instead of testing each value directly this PR tests the regressed value. This will allow more values and only fail on the ones that GraphQL can not deal with.

Why is it needed?

It helps to prevent crashing Strapi at the bootstrap phase, after a new invalid value has been added to an enum field in the admin UI.

How to test it?

  1. Navigate to CTB
  2. Add a new enum field to a content-type
  3. Add invalid values (such as --1)
  4. Validate a frontend validation error is shown

Related issue(s)/PR(s)

Co-authored-by: omacovei <omacovei@users.noreply.github.com>
@gu-stav gu-stav added source: core:content-type-builder Source is core/content-type-builder package pr: fix This PR is fixing a bug labels Jul 7, 2022
@gu-stav gu-stav added this to the 4.2.3 milestone Jul 7, 2022
@gu-stav gu-stav requested review from petersg83 and Convly July 7, 2022 11:51
@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #13717 (5be82a4) into master (92558c8) will decrease coverage by 5.61%.
The diff coverage is 0.00%.

❗ Current head 5be82a4 differs from pull request most recent head 87435f5. Consider uploading reports for the commit 87435f5 to get more accurate results

@@            Coverage Diff             @@
##           master   #13717      +/-   ##
==========================================
- Coverage   54.33%   48.72%   -5.62%     
==========================================
  Files        1197      216     -981     
  Lines       30591     8464   -22127     
  Branches     5562     1916    -3646     
==========================================
- Hits        16622     4124   -12498     
+ Misses      12160     3563    -8597     
+ Partials     1809      777    -1032     
Flag Coverage Δ
front ?
unit 48.72% <0.00%> (+0.16%) ⬆️

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

Impacted Files Coverage Δ
...ontent-manager/server/controllers/content-types.js 29.62% <0.00%> (-1.14%) ⬇️
packages/core/database/lib/migrations/index.js 30.00% <0.00%> (ø)
packages/core/database/lib/query/helpers/where.js 10.22% <0.00%> (ø)
...nents/DynamicTable/CellContent/utils/hasContent.js
...ntent-manager/components/PreviewWysiwyg/Wrapper.js
...t-manager/hooks/useFetchContentTypeLayout/index.js
...dmin/admin/src/translations/languageNativeNames.js
...admin/src/components/FormModal/attributes/types.js
...pe-builder/admin/src/components/FormModal/index.js
...UploadAssetDialog/AddAssetStep/FromComputerForm.js
... and 975 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92558c8...87435f5. Read the comment docs.

Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

Thanks for the change. It looks like we're also operating the doesNotStartWithANumber check in the content-type domain code.
Should we add some consistency between both the front & back validations?

@gu-stav
Copy link
Contributor Author

gu-stav commented Jul 7, 2022

@Convly I think it's a good idea! I've added the GraphQL enum check and another for empty values - duplicates are checked already.

I've changed the empty values check from checking the raw values to checking the regressed values, as this is the behavior I want I think.

Does that work for you?

@gu-stav gu-stav requested a review from Convly July 7, 2022 14:15
Convly
Convly previously requested changes Jul 11, 2022
Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

Thanks for the update, I just have some questions

@@ -24,6 +24,9 @@ const LIFECYCLES = [
'afterDeleteMany',
];

// See GraphQL Spec https://spec.graphql.org/June2018/#sec-Names
const GRAPHQL_ENUM_REGEX = new RegExp('^[_A-Za-z][_0-9A-Za-z]*$');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better if we're talking about the enum regexp instead?
I would prefer we avoid coupling/referencing GraphQL to core code

Copy link
Contributor Author

@gu-stav gu-stav Jul 11, 2022

Choose a reason for hiding this comment

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

I agree, but on the other hand the coupling with GraphQL is the reason why need to this. Therefore I'd prefer to explicit here and to explain why ...

Would it help if I expand on the comment?

Copy link
Member

Choose a reason for hiding this comment

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

Yep might be a good idea to explain a bit more why we're referencing GraphQL here?

Copy link
Contributor Author

@gu-stav gu-stav Jul 12, 2022

Choose a reason for hiding this comment

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

Added in f880614 Does that work for you, @Convly?

Copy link
Member

Choose a reason for hiding this comment

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

Ah the good old chicken 🐓 vs egg 🥚 issue.

@gu-stav gu-stav requested a review from Convly July 11, 2022 12:06
@pwizla
Copy link
Contributor

pwizla commented Jul 12, 2022

Please let me know when this one is merged so that I can merge documentation PR #984 😊 — if it's still worth it.

@gu-stav
Copy link
Contributor Author

gu-stav commented Jul 12, 2022

@pwizla I will. Doing my best to get this one merged, so that we do not have to introduce the disclaimer :)

Co-authored-by: Pierre Noël <petersg83@users.noreply.github.com>
Co-authored-by: Pierre Noel <pierre.noel@strapi.io>
Copy link
Contributor

@petersg83 petersg83 left a comment

Choose a reason for hiding this comment

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

LGTM :)

@gu-stav gu-stav dismissed Convly’s stale review July 18, 2022 10:05

Requested changes have been addressed

@gu-stav gu-stav merged commit 80c1da6 into master Jul 18, 2022
@gu-stav gu-stav deleted the fix/enum-ui-validation branch July 18, 2022 10:05
@gu-stav gu-stav modified the milestones: 4.2.4, 4.3.0 Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:content-type-builder Source is core/content-type-builder package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enumeration caused Strapi to crash and wipe all data
6 participants