#Issue 66 - OK to merge #67
Conversation
1 similar comment
| lib = get_lib(holding_location.locations_library_id) | ||
| if lib.code == 'firestone' | ||
| @@locator_url + "loc=" + @params[:loc]+ "&id=" + @params[:id] | ||
| else |
There was a problem hiding this comment.
The old voyager system is hardcoded to provide a link to a map for every item regardless of location. In OL we only show the map link for accessible locations. Given that the else clause here should probably redirect you to something useful for the ID like the request system page for the material. The elseif clause can check to see if the location is in a stackmapable library. The fallback else statement can send the user to https://pulsearch.princeton.edu/requests/{bib_id}.
There was a problem hiding this comment.
Ah, yes, there are three scenarios. So, is a holding Location considered accessible if it's "open"? (i.e., holding_location.open)
There was a problem hiding this comment.
Yes, in theory all the locations in off-site facilities and closed stack locations should have that flag return false.
There was a problem hiding this comment.
Might be a few exceptions. Some Marquand locations are technically "accessible" to users but are not flagged as open because it is a non-circulating library. I think the toggle value to use here is just what library the location supplied is attached to.
| def index | ||
| @map = Map.new(map_params) | ||
|
|
||
| if !@map.url.nil? |
There was a problem hiding this comment.
The logic here feels a little confusing, and violates law of demeter. What's really being checked here? If the map is valid? Can that be extracted to unless @map.valid?
| @@locator_url = "http://library.princeton.edu/locator/index.php?" | ||
| @@stackmap_url = "http://princeton.stackmap.com/view/?" | ||
| @params = params | ||
| @holding_location = set_holding_location(@params[:loc]) |
There was a problem hiding this comment.
More typical ruby-ism would be
self.holding_location = params[:loc]
| end | ||
|
|
||
| def url | ||
| if @valid |
There was a problem hiding this comment.
Why not just use the is_valid method and not actually set @Valid?
| callno = self.bibrec['call_number_display'].first | ||
| end | ||
| URI.encode(@stackmap_url + "callno=" + callno + "&location=" + self.loc + "&library=" + self.lib.label) | ||
| else |
There was a problem hiding this comment.
@sdellis I don't see a test for the reserves use case. Sorry if I missed it. I think adding this would make coveralls happy.
|
Looks good to me, just need to get coverage to 💯 |
|
Pushed a few commits to this branch.
|
|
Closes issue #66. |
|
👍 nice |
@kevinreiss Is this along the lines of what you were thinking? (Still need to write the tests for this and allow URLs to be managed through a config file.)