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

[WIP] Collection search builder part2 #1110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atz
Copy link
Contributor

@atz atz commented Nov 29, 2016

Retargeted PR against master after part1 #1105 merged.

@atz atz force-pushed the collection_search_builder_part2 branch from ad1126d to 776875e Compare November 29, 2016 00:43
@atz atz changed the title Collection search builder part2 [WIP] Collection search builder part2 Nov 29, 2016
builder.discovery_perms = access_levels[access_level] if access_level
end
collections_search_builder_class.new(self, access_levels[access_level])
# collections_search_builder_class.new(self, nil).tap do |builder|
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this stuff if it's no longer needed.

@rows = 100
super(context)
@discovery_permissions ||= access_levels[access] if access
Copy link
Contributor

Choose a reason for hiding this comment

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

@discovery_permissions is always nil here, so why use ||=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily. If this class is extended and that class' initializer sets the @discovery_permissions before calling this one, then we keep those (e.g. bypassing access_levels). But you are right we can probably do without it.

You can mostly ignore this PR while it is still WIP.

Used to un-protect the method, but no longer needed.
@atz atz force-pushed the collection_search_builder_part2 branch 4 times, most recently from 5ffbd9a to 61b8d5c Compare November 29, 2016 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants