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

Filter app results by title #4630

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

Conversation

amcaplan
Copy link
Contributor

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
  2. Create several apps with different names so you can see the search working
  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
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

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.71% (+0.05% 🔼)
8541/11747
🟡 Branches
69.64% (+0.01% 🔼)
4191/6018
🟡 Functions
71.81% (+0.08% 🔼)
2211/3079
🟡 Lines
73.04% (+0.05% 🔼)
8085/11069
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / ConcurrentOutput.tsx
98.39% (-1.61% 🔻)
90.91% (-4.55% 🔻)
100%
98.33% (-1.67% 🔻)

Test suite run success

1942 tests passing in 876 suites.

Report generated by 🧪jest coverage report action from 378d9d0

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

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.

2 participants