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.

Sign up for GitHub

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

Jump to bottom

Return API Users with distinct result when using Ransack #3674

Merged
merged 2 commits into from
Jul 14, 2020
Merged

Return API Users with distinct result when using Ransack #3674

merged 2 commits into from
Jul 14, 2020

Conversation

hefan
Copy link
Contributor

@hefan hefan commented Jun 21, 2020

Description
Return API Users ransack searches with a distinct result.

Fixes #3183
-->

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)
  • I have attached screenshots to this PR for visual changes (if needed)

@kennyadsl kennyadsl changed the title return api users ransack search with distinct result Return API Users with distinct result when using Ransack Jun 22, 2020
kennyadsl
kennyadsl approved these changes Jun 22, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍 Works for me, thanks!

jarednorman
jarednorman reviewed Jun 22, 2020
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Should there be a spec for this?

@hefan
Copy link
Contributor Author

hefan commented Jun 22, 2020

I was not quite sure to write one.
The Request Spec for spree/api/users_controller has no scenario for that.
Should i write one which tests that each user is only once returned when using a ransack search like the one in #3183?
Can you point me to where there is a similar test like this so i could follow the best practices from the start? And where the test should go to (i guess only to requests/spree/api/users_controller_spec.rb)?

@jarednorman
Copy link
Member

Yeah, I think a request spec in the file requests/spree/api/users_controller_spec.rb would be appropriate. Even just a single spec that fails without this change and passes with it, showing that the endpoint returns non-duplicated users would be great.

@spaghetticode
Copy link
Member

I was wondering if having distinct results may be something we may want to move upstream, ie. avoid that any subclass of Spree::Api::ResourceController returns duplicates, not only this one.

@kennyadsl
Copy link
Member

@spaghetticode this was the approach used in another PR for the same issue. Please see this comment #3195 (comment) and let me know any thoughts on that.

@hefan
Copy link
Contributor Author

hefan commented Jun 24, 2020 •
edited
Loading

I think about the considerations from @kennyadsl in #3195:

  1. i do not understand why not and how does distinct: true as ransack option does the same like the activerecord distinct. But i assume the activerecord distinct is not a problem from what i read there.
  2. The special handling in the backend users controller meanwhile is gone in master.

However, about the use in the resource controller i am not sure. i imagine scenarios where one could want none distinct results when searching for product/variants or orders/line items etc.

My first idea was to just remove
line 31: firstname_or_lastname_start: term here

solidus/backend/app/assets/javascripts/spree/backend/user_picker.js

Line 31 in 16ed773

firstname_or_lastname_start: term

because i never would search for an user by an adress when i want to give promotions on a user basis. Its the only place where the user picker is used.

Please tell me about the preferred way to go here..

@hefan hefan force-pushed the promotion_user_picker branch from 3ca901b to 12944a1 Compare June 30, 2020 17:18
@hefan
Copy link
Contributor Author

hefan commented Jun 30, 2020

OK. i've added a spec test for the original approach of this PR with a distinct API Users ransack search result for users

jarednorman
jarednorman approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Cool, works for me.

aldesantis
aldesantis requested changes Jul 1, 2020
View reviewed changes
api/spec/requests/spree/api/users_controller_spec.rb Outdated
@@ -144,6 +144,21 @@ module Spree
expect(response.status).to eq(422)
expect(json_response).to eq({ "error" => "Cannot delete record." })
end

it "returns distinct search results" do
uuser = create(:user, email: '[email protected]')
Copy link
Member

Choose a reason for hiding this comment

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

Small typo here, should be user?

Copy link
Contributor Author

@hefan hefan Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It was meant to be an abbreviation of unique_user. i have changed the spec to be more precise.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it! Yeah, I think the full version is more easily understandable.

@hefan hefan force-pushed the promotion_user_picker branch from 5bd9e61 to 42a5d46 Compare July 1, 2020 11:51
jarednorman
jarednorman approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Nice, I like that better as well.

@spaghetticode
Copy link
Member

@hefan can you please rebase with master? This build is failing due to an annoying flaky spec that was fixed recently. After the rebase your build should become green 💚 ... thanks 🙏

@hefan hefan force-pushed the promotion_user_picker branch from 42a5d46 to fb65b54 Compare July 10, 2020 16:03
@hefan
Copy link
Contributor Author

hefan commented Jul 10, 2020

done

@kennyadsl kennyadsl merged commit 1aa0288 into solidusio:master Jul 14, 2020
@kennyadsl
Copy link
Member

Thank you Stefan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarednorman jarednorman jarednorman approved these changes

@aldesantis aldesantis aldesantis approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
changelog:solidus_api Changes to the solidus_api gem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User picker proposes the same users at promotion edit page
5 participants
@hefan @jarednorman @spaghetticode @kennyadsl @aldesantis