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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 132 commits into from
Apr 6, 2022
Merged

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

merged 132 commits into from
Apr 6, 2022

Conversation

jshamble
Copy link
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
Copy link

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 calebcordry and alanorozco 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
@lgtm-com
Copy link

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

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
examples/amp-story/shopping/product.schema.json Show resolved Hide resolved
examples/amp-story/shopping/product.schema.json Outdated Show resolved Hide resolved
examples/amp-story/shopping/product.schema.json Outdated Show resolved Hide resolved
examples/amp-story/shopping/product.schema.json Outdated Show resolved Hide resolved
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js Outdated Show resolved Hide resolved
src/url.js Outdated Show resolved Hide resolved
extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated Show resolved Hide resolved
extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated Show resolved Hide resolved
extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

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

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
@lgtm-com
Copy link

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

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 reviewed Apr 6, 2022
View reviewed changes
src/url.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

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

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

@processprocess processprocess processprocess approved these changes

@coreymasanto coreymasanto coreymasanto approved these changes

@gmajoulet gmajoulet gmajoulet approved these changes

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