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 requests by folder, URI, method, query string #797

Merged

Conversation

shawnaxsom
Copy link
Contributor

Relevant issues?

#739 - This fulfills at least some part of the issue. We may want to update the UI to display either what parts of text are matched, and/or show missing fields (e.g. add URL to the Quick Switch modal).

What does this PR do?

  • Adds a fuzzyMatchAll method to simplify and standardize how multiple-field searches are performed.
  • Applies fuzzyMatchAll to request search bars in the Sidebar and in the Quick Switch modal.
  • Updates those request search bars to have more descriptive placeholders.
  • Adds unit tests for both fuzzyMatch and fuzzyMatchAll.

What is left to do or discuss on the relevant issues?

  • We might want to make Quick Switch window display URI, 2+ folders (all parents), possibly query string. Either multiple rows, and/or show the field that matched. It might be a little much to include many e.g. query string parameters on there, so some things might have to stay hidden, or we have to get creative.
  • Should we keep Fuzzy Matching as implemented? An option to turn it off might be nice. I was wanting to search for POST earlier, and it was matching something like "httP://lOcalhoST". I prefer multiple word non-fuzzy searching for this reason. But I assume others prefer fuzzy matching or it wouldn't be there.
  • If we keep Fuzzy Matching as implemented, consider whether labels are needed. Probably overkill, but we could add optional labels to the search, like "method:POST url:localhost".

Example screenshots, gifs, or videos?

What code can others reuse?

  • fuzzyMatchAll - This calls out to the existing fuzzyMatch misc utility method. The difference is that it takes a string that will be split on spaces for required search terms, and it takes an array of fields that the search terms can match against.

Testing Steps

  • Search for URI, method, query string, folder, and/or request name on Quick Switch window
  • Perform fuzzy search, leaving out some letters for a given field
  • Perform a multiple word search, searching multiple types of fields in the list above
  • Repeat tests for Sidebar search

@welcome
Copy link

welcome bot commented Mar 5, 2018

💖 Thanks for opening this pull request! 💖

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

@gschier
Copy link
Contributor

gschier commented Mar 8, 2018

Haven't forgot about this! Should be able to get to it next week 😄

Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

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

Nice work @axs221! Only a few minor comments.

* Appends path of ancestor groups, delimited by forward slashes
* E.g. Folder1/Folder2/Folder3
*/
_groupOf (request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing. This variable would be more accurately described as child or requestOrRequestGroup. Can you rename to be more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll go with requestOrRequestGroup, verbose but clear as what can be expected

request.url,
request.method,
this._groupOf(request),
...(request.parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be more intuitive to join the parameters to the URL? Thoughts?

See https://github.com/getinsomnia/insomnia/blob/cd1ff32b4d098979cc61de6837475f648bda82e3/plugins/insomnia-plugin-request/index.js#L108-L109 for an example of URL building.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I will change it as suggested

@gschier
Copy link
Contributor

gschier commented Mar 12, 2018

Responding to your comments in the PR description:

We might want to make Quick Switch window display URI, 2+ folders (all parents), possibly query string. Either multiple rows, and/or show the field that matched. It might be a little much to include many e.g. query string parameters on there, so some things might have to stay hidden, or we have to get creative.

I agree, it's probably best to show as much as possible. Perhaps we could hide most things by default and only show them if they were part of the match?

Should we keep Fuzzy Matching as implemented? An option to turn it off might be nice. I was wanting to search for POST earlier, and it was matching something like "httP://lOcalhoST". I prefer multiple word non-fuzzy searching for this reason. But I assume others prefer fuzzy matching or it wouldn't be there.

I agree that the fuzzy search right now is probably too loose, specifically for the sidebar filter. For the request switcher, the current solution would probably be fine if results were sorted by best match like in Atom/Sublime/etc.

If we keep Fuzzy Matching as implemented, consider whether labels are needed. Probably overkill, but we could add optional labels to the search, like "method:POST url:localhost".

Oh, that's interesting. Like GitHub does with issues. I think labels are a great idea either way because we could add things like status:200, auth:Basic, content-type:json, etc.

Thanks @axs221!

@shawnaxsom shawnaxsom force-pushed the feature/fuzzy-match-request-parameters branch from caa89fc to 58e00cc Compare March 20, 2018 00:11
@shawnaxsom
Copy link
Contributor Author

@gschier Okay I updated the branch with the minor feedback from the PR, minus the items mentioned in Responding to your comments in the PR description.

Is what is there so far worth merging, and then doing a second PR for the rest? Otherwise I should be able to get to the additional items in the next week or so.

Thank you!

@gschier
Copy link
Contributor

gschier commented Mar 28, 2018

Yep, it looks great!

@gschier gschier merged commit bee9973 into Kong:develop Mar 28, 2018
@gschier
Copy link
Contributor

gschier commented Mar 28, 2018

I was busy for the past couple weeks traveling but will now be more available and responsive to help out if needed 😄

@shawnaxsom
Copy link
Contributor Author

@gschier Okay thanks! Hope you had a good time traveling; I've been busy too, no worries. I'll put some thought into the UI changes soon and we can discuss.

@gschier
Copy link
Contributor

gschier commented Mar 28, 2018

It was amazing, thanks for asking 😃

Sounds good! Probably best to create a new issue for discussion of the next phase 👍

@gschier
Copy link
Contributor

gschier commented Mar 29, 2018

@axs221 just so you're aware, I'm disabling the new fuzzy matching for URL/parameters until we have some UI to make it more clear what's going on.

luizmariz pushed a commit to luizmariz/insomnia that referenced this pull request Jan 22, 2020
* Quick Switch matching for Request URL and Method

Previously only Request Name was searched for in Quick Switch window.
This adds support for searching Request URL and Method as well.
A fuzzyMatchAll function has been added to be able to search different
fields in any
order, space delimited.

* Include request parameters in searchable fields

* Allow searching requests by folder paths

* More descriptive placeholder for Quick Switch modal search input

* Update sidebar filter to match Quick Switch, allowing URL and Query String matching

* More descriptive placeholder for sidebar search

* Unit tests for fuzzyMatch and fuzzyMatchAll

* More unit tests for fuzzyMatch and fuzzyMatchAll

* minor refactorings
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

2 participants