Category browsing: not filtering category anymore

AronVanAmmers commented 8 years ago

Probably lost with introduction of filter

bartvanderwal commented 8 years ago

I thought you meant browsing on the homescreen but that is working on my machine:

Did you mean something else, or did you already fix it @AronVanAmmers?

AronVanAmmers commented 8 years ago

I meant that yeah, in my case I clicked Camera and ended up with everything. Will retest.

Aron van Ammers

CTO and Founding Partner Blockstars.io

S: aronvanammers | T: @aronvanammers | L: http://nl.linkedin.com/in/aronvanammers On Feb 2, 2016 9:44 PM, "Bart van der Wal" notifications@github.com wrote:

I thought you meant browsing on the homescreen https://selfsigned.blockstars.io:4124/ but that is working: [image: image] https://cloud.githubusercontent.com/assets/3029472/12768315/291fed36-ca0f-11e5-8e9f-43c9a7c6e41a.png

Did you mean something else, or did you already fix it @AronVanAmmers https://github.com/AronVanAmmers?

— Reply to this email directly or view it on GitHub https://github.com/OutlierVentures/BuyCo/issues/33#issuecomment-178891643 .

bartvanderwal commented 8 years ago

Ah no, I have the same behaviour. I tested with only one proposal in system, but by now managed to populate with many using the seed method. So I understand now, you meant the filtering is not effective, e.g. when you try to apply filter for one (sub)category it still returns you all proposals instead, regardless of their category.

bartvanderwal commented 8 years ago

Fixed.

Issue was that I assumed we'd use URL parameters for all filter parameters for proposals. I had not noticed that @AronVanAmmers had neatly defined sections of the URL path to be used for the sub- and maincategory in routes/api.ts: /proposal/category/:mainCategory/:subCategory

So now we use those again. It's nicer for those users that practice URL hacking and even SEO if this were ever to be moved out from behind login.

Puttin all filter parameters in the URL path now would be taking it to far, specifically because the other parameters don't have the logical order in the URL that sub- and main category have. So they are still expected to be in the Querystring. I've documented it in the comments.

// Determine the applicable filters from the request URL - if any.
// Example https://selfsigned.blockstars.io:4124/proposal/category/Books/Fantasy%20books?maxPrice=2&minimumTotalAmount=250
// Most filter parameters are taken from the URL Query (e.g. URL parameters) api/
..
// Only the maincategory and subcategory are in the URL part itself.
...

bartvanderwal commented 8 years ago

Ah wait no, in the previous comment I was mixing up the API URL with the regular URL, meant for humans. I thought about it a while, see below. For now I'm leaving it as it is, as everything works.

Personally - given time - right now I'd be in favor of changing the js/categories/categoryController to use URL query for filter params instead of the path, instead of the change I now did. That controller could be refactored to move it's AJAX call logic to a DI'dproposalService anyway.

TLDR

For the API URL I don't know a good reason why we should use the URL path instead of query for filter parameters. And it's a bit strange to use one for some and the other for some others, as it is now.

The current code actually allows both styles for the category filters (both in URL path or in URL query). Because the category-screens use the URL path and works and the seller-proposals-screen uses the URL query, and still works. But it would be clearer to choose one.

Advantage of using URL path:

The parameters are explicitly named and defined in the routes. So you can see them there as some sort of documentation.
It is inline with the :id used in a lot of other places.

The disadvantage of using URL path;

The parameters are now explicitly named and defined in the routes :P. So you have to (also) make changes in the router logic if you ever rename, add or delete filter criteria. Not DRY.
Having 4 or more URL parts the URL's get sort of long.
To prevent the previous you can mix both as now done, but than you become internally inconsistent as already argued.

app.routes.ts:

.when('/proposal/category/:mainCategory/:subCategory', { controller: Proposa...

api.ts:

apiRouter.route("/proposal/category/:mainCategory/:subCategory").get(pc.get);

OutlierVentures / BuyCoPoc