Skip to content

✨ JSON validation for amp-story-shopping config#37474

Merged
jshamble merged 132 commits intoampproject:mainfrom
jshamble:shoppingJSONValidation
Apr 6, 2022
Merged

✨ JSON validation for amp-story-shopping config#37474
jshamble merged 132 commits intoampproject:mainfrom
jshamble:shoppingJSONValidation

Conversation

@jshamble
Copy link
Copy Markdown
Contributor

@jshamble jshamble commented Jan 25, 2022 •
edited
Loading

This PR adds scalable validation for the amp-story-shopping config, see the i2i here for a complete list of required and optional fields, utilizing ajv and a JSON schema which runs on each item in the config.: #36460

The validation will not render shopping tags in the page or in the PLP if a required field is not found or is invalid.
However, it will still render valid shopping tags on the page. Blocking the rendering is done per tag, not per page.

@amp-owners-bot amp-owners-bot bot requested a review from mszylkowski January 25, 2022 19:19
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Jan 25, 2022 •
edited
Loading

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/amp-story-shopping-config.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js

@jshamble jshamble marked this pull request as draft January 25, 2022 19:19
@jshamble jshamble requested review from alanorozco and calebcordry and removed request for mszylkowski January 25, 2022 19:38
@gmajoulet gmajoulet mentioned this pull request Jan 27, 2022
Closed
@jshamble jshamble marked this pull request as ready for review January 28, 2022 10:24
@jshamble jshamble self-assigned this Jan 29, 2022
@jshamble jshamble requested a review from coreymasanto January 31, 2022 18:55
processprocess
processprocess reviewed Jan 31, 2022
View reviewed changes
jridgewell
jridgewell approved these changes Apr 4, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Owners approval

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 4, 2022

This pull request introduces 1 alert when merging 85151a8 into 2091f72 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 4, 2022

This pull request introduces 1 alert when merging 4227e69 into a61e479 - view on LGTM.com

new alerts:

  • 1 for Syntax error

alanorozco
alanorozco requested changes Apr 4, 2022
View reviewed changes
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 5, 2022

This pull request introduces 1 alert when merging 03e0b27 into a61e479 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@jshamble jshamble requested a review from alanorozco April 5, 2022 00:39
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 5, 2022

This pull request introduces 1 alert when merging 66dafaa into a61e479 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@jshamble jshamble mentioned this pull request Apr 5, 2022
Merged
processprocess
processprocess reviewed Apr 5, 2022
View reviewed changes
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 5, 2022

This pull request introduces 1 alert when merging bf8bd53 into c9b09d0 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 5, 2022

This pull request introduces 1 alert when merging 8204ae9 into d0dede1 - view on LGTM.com

new alerts:

  • 1 for Syntax error

alanorozco
alanorozco approved these changes Apr 6, 2022
View reviewed changes
alanorozco
alanorozco reviewed Apr 6, 2022
View reviewed changes
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 6, 2022

This pull request introduces 1 alert when merging 656f63e into cb67fc9 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Apr 6, 2022

This pull request introduces 1 alert when merging dbf5af0 into cb67fc9 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@jshamble jshamble merged commit e5cac4a into ampproject:main Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alanorozco alanorozco alanorozco approved these changes

@jridgewell jridgewell jridgewell approved these changes

@gmajoulet gmajoulet gmajoulet approved these changes

@coreymasanto coreymasanto coreymasanto approved these changes

@processprocess processprocess processprocess approved these changes

@calebcordry calebcordry Awaiting requested review from calebcordry

@mszylkowski mszylkowski Awaiting requested review from mszylkowski

Assignees

@jshamble jshamble

Labels

PR use: In Beta / Experimental PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jshamble @jridgewell @alanorozco @gmajoulet @coreymasanto @processprocess @calebcordry @mszylkowski @ampprojectbot