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

Handle queries (i.e. "?name=Pikachu") #23

Closed
seriema opened this issue Jul 22, 2015 · 5 comments
Closed

Handle queries (i.e. "?name=Pikachu") #23

seriema opened this issue Jul 22, 2015 · 5 comments

Comments

@seriema
Copy link
Owner

seriema commented Jul 22, 2015

Currently query parameters in the API URL are just stripped out. There should be an optional setting for including them in the redirect.

Use case
Better testing of search-like features where one could test search terms that will give different search results, such as no results, a few results, a lot of results, spelling suggestions, etc.

Usage
Config set in the config() by setting stripQueries to false (default is true). Sample:

angular.module('testApp', ['apiMock'])
.config(function (apiMockProvider) {
  apiMockProvider.config({
    stripQueries: false
  });
});

Descoped (do this later as a separate feature):
Local and global flag set on individual requests, which would be perfect for isolated testing of a autocompletion type search. To not pollute the $http config we should scope it within apiMock, but that requires another feature (to allow config objects instead of just true/false). Sample:

app.service('exampleService', function ($http) {
  // ...
  $http({
    method: 'GET',
    url: '...',
    apiMock: {
      stripQueries: true
    }
  }) // ...
  // ...
});

Effect

  • Changes redirect logic so that /api/pokemon?name=Pikachu no longer strips the query and redirects to /mock_data/pokemon.get.json. Instead it redirects to /mock_data/pokemon/name=pikachu.get.json (similar to mock url #13 where /api/pokemon would redirect to /mock_data/pokemon/get.json instead of /mock_data/pokemon.get.json, but for now we only do this for URL's with queries).
  • It will lowercase and URI encode the queries (i.e. Pikaču becomes pika%C4%8Du).
  • It will sort the query params in alphabetical ascending order (i.e. strength=...&name=... becomes name=...&strength=...).
@trevcor
Copy link

trevcor commented Jul 28, 2015

Still trying to steal some time away to work on this. Hopefully be done this week.

@seriema
Copy link
Owner Author

seriema commented Jul 28, 2015

No problem! I'd love to hear your comments on the questions above.

@trevcor
Copy link

trevcor commented Jul 30, 2015

Had some time to look around more of the code and think about your questions.

  1. I agree we should lowercase the query values by default but give a config to override that behavior. I think the query param name should stay as it is. Should we call the config forceLowerCaseQueries and default to true?
  2. Yep I like this pattern way better than the underscore.
  3. Agreed. Default to true makes more sense.
  4. Yeah this makes sense.
  5. I'm still looking at that commit. When I view it in github it looks like the entire file was changed so its tough to see what exactly the change was.
  6. I also agree with this. Let's get the global stuff working and then we can add the targeted api feature later.

So for the global feature we will add 2 config options, forceLowerCaseQueries, stripQueries.

Anything else?

@seriema
Copy link
Owner Author

seriema commented Jul 30, 2015

  1. If the request comes up we can add the flag. I can't find a good use case for mixed casings and it would just add more code/tests unnecessarily. For now let's just have lowercase as default.
  2. Yeah it's annoying (hence why I request clean diffs in the contribution guidelines now), but ctrl+f for "appendParamsToPath" and you should see it.

Thanks!

seriema added a commit that referenced this issue Aug 6, 2015
feat(apimock):  add query param functionality

Close #23
seriema added a commit that referenced this issue Aug 6, 2015
seriema added a commit that referenced this issue Aug 6, 2015
We now handle URL query parameters. Ref #23
seriema added a commit that referenced this issue Aug 6, 2015
…aram

$http’s config.param can include nested objects and arrays, but parsing
them to a valid file path will have to wait. So they’re not being
included in the path for now.

Ref #23
@seriema
Copy link
Owner Author

seriema commented Aug 9, 2015

If anyone wants to discuss this feature, I've opened a Gitter chatroom.

seriema added a commit that referenced this issue Oct 18, 2015
From 97.2% to 100%.

* Removed unnecessary guard clauses in things I borrowed from AngularJS
source code (mostly for IE8 support).
* Used $filter instead of manually removing elements from array in
removeFallback().
* Added tests to better flex the query handling feature in #23.
** Testing array of objects.
** Undefined and empty URL param values.
** Date-type in params object.

Closes #44
seriema added a commit that referenced this issue Oct 18, 2015
From 97.2% to 100%.

* Removed unnecessary guard clauses in things I borrowed from AngularJS
source code (mostly for IE8 support).
* Used $filter instead of manually removing elements from array in
removeFallback().
* Added tests to better flex the query handling feature in #23.
** Testing array of objects.
** Undefined and empty URL param values.
** Date-type in params object.

Closes #44
seriema added a commit that referenced this issue Oct 18, 2015
From 97.2% to 100%.

* Removed unnecessary guard clauses in things I borrowed from AngularJS
source code (mostly for IE8 support).
* Used $filter instead of manually removing elements from array in
removeFallback().
* Disable video and screenshots on SauceLabs as the tests aren't visual.
* Added tests to better flex the query handling feature in #23.
** Testing array of objects.
** Undefined and empty URL param values.
** Date-type in params object.

Closes #44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants