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

Add a search interface for orders on an user's admin page #25

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

Conversation

aitbw
Copy link

@aitbw aitbw commented Apr 11, 2019

Description

This PR adds a search interface for orders under admin/users/:id/orders; it was a pending TODO as per this comment.

It emulates, til some point, the functionality under admin/orders. While it takes most of the fields used on that view, it does not consider others, as the modified view is limited in space and I think it's for the best to keep it as clean and concise as possible.

I also think, given the context, that admins might be not be interested in such specific details under a particular user.

Given the similarities between this view/controller action and the one for admin/orders, I think there might be room for a refactor, to DRY things up a little bit, but that'd be included on another PR if this one gets approved for Solidus.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

@kennyadsl
Copy link
Member

I'd probably have removed the TODO honestly. It's a lot of duplicated code for a feature that I feel like it's not used a lot. Not sure what to do at the moment.

@aitbw
Copy link
Author

aitbw commented Apr 15, 2019

Maybe we could apply a refactor first —extract view/action logic, make it as agnostic as possible and then add the search functionality to the related views, WDYT?

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