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

Filter app results by title #4630

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Filter app results by title #4630

wants to merge 2 commits into from
+187 −85

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Oct 10, 2024 •
edited
Loading

WHY are these changes introduced?

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

Pairs with https://github.com/Shopify/shopify/pull/539421 to enable filtering by app title in the new API.

Note this should NOT be merged until that PR is merged.

WHAT is this pull request doing?

Updates the format of the request to work with the new paginated field, and passes in the search term as a query for app title.

Screenshot 2024-10-09 at 20 26 55 Screenshot 2024-10-09 at 20 27 18

Note that each token is considered an independent search term, so we're searching for an app title that includes all mentioned tokens.

Screenshot 2024-10-09 at 20 28 01

How to test your changes?

  1. Point the new API to the filter-using-pagination-system branch (update as of 2024-11-14: this works on main or dd as well)
  2. Create several apps with different names so you can see the search working (not necessary if pointing to production)
  3. Run USE_APP_MANAGEMENT_API=1 bin/spin pnpm shopify app config link --path /PATH/TO/YOUR/APP
  4. Select the option to connect to an existing app, and search for different app titles.

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

@amcaplan amcaplan requested a review from a team as a code owner October 10, 2024 12:11
@amcaplan amcaplan requested review from gordonhirsch and alexanderMontague and removed request for a team October 10, 2024 12:11
@github-actions GitHub Actions
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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 10, 2024 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
74.43% (+0.08% 🔼)
 8498/11418
🟡 Branches
70.18% (+0.05% 🔼)
 4139/5898
🟡 Functions
73.9% (+0.03% 🔼)
 2237/3027
🟡 Lines
74.94% (+0.07% 🔼)
 8037/10724
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / migrate-app-module.ts
89.19% (-0.28% 🔻)
 85.71% 90.91%
91.18% (-0.25% 🔻)
🔴
... / system.ts
33.33% (-1.55% 🔻)
21.43% (-1.9% 🔻)
62.5% (-4.17% 🔻)
34.29% (-1.43% 🔻)

Test suite run success

1941 tests passing in 881 suites.

Report generated by 🧪jest coverage report action from 4de2e9f

@MitchDickinson
Copy link
Contributor

CC @amcaplan

Note that as part of https://github.com/Shopify/shopify/pull/539421, I introduced the connection API but I also preserved the old apps field on the GraphQL schema. So now we have two ways to access apps:

  • appsConnection
  • apps
    I did not want to have time pressure to ship this CLI PR while the CLI was in a broken state, so I took the safe path.

This PR is probably broken if it's using apps and assuming it's getting back a connection. I think the work outstanding here is going to be (in order to never break the latest CLI):

  1. Ship this PR targeting appsConnection
  2. If you want to rename appsConnection
  • Update apps to also be a connection type
  • Update CLI to use apps
  • Remove appsConnection from server

Or if you feel comfortable moving fast and breaking things, go ahead. I'm not opposed to that with good comms around it in our project channel. I just didn't want to take that on myself :)

@MitchDickinson
Copy link
Contributor

@amcaplan should we aim to get this shipped?

@amcaplan amcaplan force-pushed the filter-app-results-on-app-title branch from 378d9d0 to 33a3c20 Compare November 14, 2024 15:43
@github-actions GitHub Actions
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.

@amcaplan amcaplan force-pushed the filter-app-results-on-app-title branch from 33a3c20 to 4de2e9f Compare November 14, 2024 15:46
@amcaplan
Copy link
Contributor Author

I think this is ready to go. Currently it points to appsConnection. We can choose to leave it as is, or update apps to be a connection as well and then port this over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gordonhirsch gordonhirsch Awaiting requested review from gordonhirsch gordonhirsch is a code owner automatically assigned from Shopify/app-inner-loop

@alexanderMontague alexanderMontague Awaiting requested review from alexanderMontague alexanderMontague is a code owner automatically assigned from Shopify/app-inner-loop

@MitchDickinson MitchDickinson Awaiting requested review from MitchDickinson

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amcaplan @MitchDickinson