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

on import link extensions make absolute urls relative #4877

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

Conversation

alfonso-noriega
Copy link
Contributor

@alfonso-noriega alfonso-noriega commented Nov 19, 2024

WHY are these changes introduced?

The current process to create legacy link extensions in the partners dashboard enables to create non secure and external pointing absolute URLs for embedded apps. Those URLs are later on overwritten by core to point to the shop embedded app URL.
This behaviour fills the DB with incorrect URLs and the new CLI managed admin link extension is avoiding this pattern.

WHAT is this pull request doing?

To reduce friction on the partners migration, this code automatically ignores the domain of the embedded app legacy link extensions and transforms them into relative URLs.

How to test your changes?

  • From the CLI repo, checkout this branch.

  • Create a link extension in the partners dashboard with an absolute URL pointing to a random external domain (http://example.org/product).

  • In the CLI repo run the import extensions command: pnpm shopify app import-extensions --path path-to-your-app

  • Select the admin links option

  • Check the the generated extension toml contains a url like app://product

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

  • I've considered possible documentation changes

@alfonso-noriega alfonso-noriega force-pushed the admin-link-import-transform-url-to-relative branch 7 times, most recently from 1b3a211 to ae2e639 Compare November 20, 2024 14:49
@alfonso-noriega alfonso-noriega force-pushed the admin-link-import-transform-url-to-relative branch 2 times, most recently from b2d26db to bd01c9b Compare November 20, 2024 15:14
Copy link
Contributor

github-actions bot commented Nov 20, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
74.53% (+0.01% 🔼)
8549/11470
🟡 Branches
70.34% (-0.06% 🔻)
4174/5934
🟡 Functions
74.01% (+0.09% 🔼)
2241/3028
🟡 Lines
75.07% (+0.03% 🔼)
8086/10771
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / extension-instance.ts
85.26% (-1.28% 🔻)
80.53% (-0.88% 🔻)
92%
86.81% (-1.39% 🔻)
🟢
... / extension.ts
85.71% (-14.29% 🔻)
0% (-100% 🔻)
75% (-25% 🔻)
85.71% (-14.29% 🔻)
🔴
... / dev-session.ts
4.21% (-0.19% 🔻)
0%
4.35% (-0.2% 🔻)
4.76% (-0.12% 🔻)

Test suite run success

1947 tests passing in 885 suites.

Report generated by 🧪jest coverage report action from 83ee53c

@alfonso-noriega alfonso-noriega marked this pull request as ready for review November 20, 2024 15:20
@alfonso-noriega alfonso-noriega requested a review from a team as a code owner November 20, 2024 15:20
Copy link
Contributor

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.

@alfonso-noriega alfonso-noriega force-pushed the admin-link-import-transform-url-to-relative branch 7 times, most recently from 95ec4f0 to 2a816ca Compare November 21, 2024 13:01
@alfonso-noriega alfonso-noriega force-pushed the admin-link-import-transform-url-to-relative branch from 2a816ca to 83ee53c Compare November 21, 2024 13:17
@alfonso-noriega
Copy link
Contributor Author

/snapit

Copy link
Contributor

🫰✨ Thanks @alfonso-noriega! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

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