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

Fix webhook link logic #3892

Merged
merged 4 commits into from
May 28, 2024
Merged

Fix webhook link logic #3892

merged 4 commits into from
May 28, 2024

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented May 13, 2024

Fixes https://github.com/Shopify/develop-app-management/issues/1792

WHY are these changes introduced?

There are two ways of defining webhooks in the toml:

  • Condensed: multiple topics in the same subscription definition (sharing the same URI)
  • Expanded: each subscription has only 1 topic

When linking to a new app we need to default to the condensed state, but right now we are doing expanded.

WHAT is this pull request doing?

  • Remove the simplify function from the specification
  • Update the webhook reverse transform to transform from expanded to condensed.
  • Add an additional transform to the link command to further condense webhooks

How to test your changes?

See the description in this issue

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

Copy link
Contributor

github-actions bot commented May 13, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
71.88% (+0.05% 🔼)
7145/9940
🟡 Branches
69.17% (+0.05% 🔼)
3529/5102
🟡 Functions
71.58% (+0.12% 🔼)
1917/2678
🟡 Lines
73.14% (+0.04% 🔼)
6723/9192
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / select-app.ts
74.19% (-0.81% 🔻)
80% (-2.35% 🔻)
88.89% (-1.11% 🔻)
76.92% (-0.85% 🔻)
🟢
... / link.ts
96.7% (+0.19% 🔼)
90.91% (-1.03% 🔻)
100%
96.59% (+0.21% 🔼)

Test suite run success

1667 tests passing in 774 suites.

Report generated by 🧪jest coverage report action from 16d806c

Copy link
Contributor

@karenxie karenxie left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for getting this resolved @gracejychang!

@@ -2828,7 +2806,7 @@ describe('WebhooksSchema', () => {
code: zod.ZodIssueCode.custom,
message: 'You can’t have duplicate subscriptions with the exact same `topic`, `uri` and `filter`',
fatal: true,
path: ['webhooks', 'subscriptions', 1, 'topics', 0, 'products/create'],
path: ['webhooks', 'subscriptions', 0, 'topics', 1, 'products/create'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: what do these numbers mean and why did they change with this PR? I'm guessing it has something to do with the sorting?

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh, I think this might have something to do with condensing the webhooks, instead of sorting? cause the subscriptions look like this after the transform on the schema:

      subscriptions: [
        {
          topics: ['products/update', 'products/update'],
          uri: 'https://example.com',
          filter: 'title:shoes',
        },
      ],

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a great DX... the number used to tell you what index of [[webhooks.subscriptions]] (ie. the order top to bottom) you should be looking at. Now it will forever be webhooks.subscriptions.0.topics.... I think we should come back to this or at least chat with UX on the best path forward.

@karenxie
Copy link
Contributor

only weirdness I saw when testing is that the the subscriptions showed 0 on the version page, but a refresh showed the subscription I was expecting. not blocking and not related to this PR since it seems to happen on main too but just noting it here

Copy link
Contributor

@alexanderMontague alexanderMontague left a comment

Choose a reason for hiding this comment

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

Not sure I agree with the approach but agreed lets not block and get this shipped! We can clean it up on the server for M2 - thanks for looking into this

@isaacroldan
Copy link
Contributor Author

Approved 👍

@gracejychang gracejychang added this pull request to the merge queue May 28, 2024
Merged via the queue into main with commit c0013a3 May 28, 2024
32 checks passed
@gracejychang gracejychang deleted the remove-webhook-simplify branch May 28, 2024 16:03
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.

None yet

4 participants