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

Please, make single-element list supporting finding record by slugs too, just as .show does #443

Open
ghost opened this issue Sep 10, 2015 · 2 comments

Comments

@ghost
Copy link

ghost commented Sep 10, 2015

Hello,
I use ActiveScaffold with one of my models, which supports finding records by slugs (via friendly_id). And I've noticed some strange behavior:
While .show works pretty well with this:

Started GET "/fremantle/resources/63429" for ::1 at 2015-09-10 23:42:40 +0300
Processing by Fremantle::Guest::ResourcesController#show as HTML
  Parameters: {"id"=>"63429"}
  Fremantle::Resource Load (0.3ms)  SELECT  "fremantle_resources".* FROM "fremantle_resources" WHERE "fremantle_resources"."slug" = ?  ORDER BY "fremantle_resources"."permalink" DESC LIMIT 1  [["slug", "63429"]]

…but .index with id (single-element list) doesn't work (doesn't find the record):

Started GET "/fremantle/resources/63429/list" for ::1 at 2015-09-10 23:43:27 +0300
Processing by Fremantle::Guest::ResourcesController#index as HTML
  Parameters: {"id"=>"63429"}
   (0.3ms)  SELECT COUNT(*) FROM "fremantle_resources" WHERE "fremantle_resources"."id" = ?  [["id", 63429]]

As ActiveScaffold already has some solution (do_show) which works well, may some part of it be ported to do_list ?

P.S. In rails console the difference is in parameter to find method:
Fremantle::Resource.find("63429") works well, while Fremantle::Resource.find(63429) doesn't work.

@scambra
Copy link
Member

scambra commented Sep 11, 2015

Right now it's using id_condition method on all_conditions, so it's adding condition to where method, and do_show uses find_if_allowed, which uses find method, not where

So changing to use find_if_allowed, like do_show, would have some behaviour changes:

  • filtering by unexisting id would give a 404, instead of an empty list like now
  • other filtering conditions wouldn't be used, although this change could be avoided if find_if_allowed is called with a scope which includes these conditions

Also, loading with a array id works, e.g. /resources?id[]=63429&id[]=63430 looks for id IN (63429, 63430), not sure if it would work for find_if_allowed, probably it does

@ghost
Copy link
Author

ghost commented Sep 11, 2015

Thanks for the explanation. Some my thoughts about it:

  • filtering by unexisting id would give a 404, instead of an empty list like now

It may be not strictly copying do_show, but also rather following it's logic (using find_if_allowed instead of where…) Probably, find_if_allowed in this case should also be modified to return only all of the allowed records (should not all of the requested ids/slugs be allowed).

What about working with array, you are right: list is about arrays, and single-element list is now treated like a particular case of list. I just wondered could this case be treated by some special do_show-like code?

The only thing I continue to warry about, is logic inconsistency in ActiveScaffold's treating of single-element lists, which breaks the "least surprise" principle of Rails and common logic of applications. I meant the following:

Given I have a model (i. e. Resource) using some kind of slugs, forming corresponding `to_param`, which  differs from `id`.
And I have some public method on this model's active-scaffolded controller (i. e. `edit`).
And I have a route for this method: `get 'edit', on: :member` (via ActiveScaffold aliases).
And I have another public method on this model's active-scaffolded controller, 
which should render 1 model instance in single-element list (it's `list`).
And I have a route for this method: `get 'index', on: :member` (via ActiveScaffold aliases).

According to routing table now we have:

resource = create(:resource, id: 5, slug: 'five')
expect(edit_resource_path(resource)).to eq('/resources/five/edit') # PASS
expect(list_resource_path(resource)).to eq('/resources/five/list') # PASS

And now is the most interesting part. We need list of the records with action_links.
Let ActiveScaffold form action_links for both methods.
It will use to_param in both cases (which I think is proper, predictable and not surprising behavior. Thank you for implementing it :))
AS just doesn't need to know and worry that to_param != id.
They will be:

/resources/five/edit
/resources/five/list

For now all seems to be OK and non-surprising. Just seems ;)
Let user follow and call both of these links. What does he expect to appear?

When I follow '/resources/five/edit' link I should see edit form for resource with id: 5 # PASS
When I follow '/resources/five/list' link I should see single-element list for resource with id: 5 # FAIL!!!
# WHY??? IT LOOKS AND SHOULD WORK THE SAME WAY!
# World should be orthogonal; things that looks similar should work similar!
# Single-element list should also be REST-ful. Why have some strange exceptions??

I probably have to agree with you, that logic in list is rather complex and changing it may be rather unwanted/hard to implement.
May it be better to change forming action_links for single-element lists to someting like
/resources/?slug[]=five or simply to use id ignoring to_param (/resources/5/list)?
(If so, please, continue to use to_param for all other actions, which use find_if_allowed logic. Just change it for list-like actions with different, where-based logic).

Or, may be, you could just advice me to make some change to my applications in part of forming action_links with proper params, which would make single-element list happy? :)

Anyway, it would be nice to have it's behavior documented in the wiki (possibly with link to this issue) for developers to know, that it has different logic and so needs different, non-REST-ful referring in path- and url-helpers to work as expected.

Still, I hope it could be implemented in some REST-ful way…

@ghost ghost changed the title Single-element list should support finding record by slugs too, just as .show does Please, make single-element list supporting finding record by slugs too, just as .show does Sep 14, 2015
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

No branches or pull requests

1 participant