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

Display field for app management errors #5368

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Feb 6, 2025

WHY are these changes introduced?

Fixes https://github.com/Shopify/develop-app-inner-loop/issues/2487

We are not properly displaying app management errors - the field is not included in the error message.

Turns out this is due to discrepancies in the error format coming from Partners vs. App Management.

WHAT is this pull request doing?

Updates the error handling so we always display the field if available.

Note much of this PR is handling that appVersion is nullable (and will be null when the version fails to be created), which we weren't allowing for previously.

How to test your changes?

  1. In a dev environment, make this change
  2. Add this content to your app TOML:
    [[webhooks.subscriptions]]
    compliance_topics = ["customers/data_request", "customers/redact", "shop/redact"]
    uri = "https://example.com"
  3. Try to deploy your app

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

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@amcaplan amcaplan requested a review from a team as a code owner February 6, 2025 13:10
Copy link
Contributor

github-actions bot commented Feb 6, 2025

We detected some changes at packages/*/src 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 Feb 6, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.61% (+0.01% 🔼)
9055/11976
🟡 Branches
70.87% (+0.02% 🔼)
4420/6237
🟡 Functions
75.4% (+0.01% 🔼)
2376/3151
🟡 Lines
76.12% (+0.02% 🔼)
8551/11233
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / deploy.ts
87.5% (+0.32% 🔼)
83.33% (-4.17% 🔻)
87.5%
89.47% (+0.28% 🔼)
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%

Test suite run success

2044 tests passing in 913 suites.

Report generated by 🧪jest coverage report action from b1e24df

@amcaplan amcaplan force-pushed the display-app-management-api-validation-errors-with-field branch from da74508 to fbdac68 Compare February 6, 2025 15:41
Also, accurately reflect nullability of app version info
@amcaplan amcaplan force-pushed the display-app-management-api-validation-errors-with-field branch from fbdac68 to b1e24df Compare February 6, 2025 16:39
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.

1 participant