Geographic search and display

[lmgonzales]

Notes:

• Geographic search code was based on Moravian Library/ Vaclay Rozceky code implementation (OpenLayers2 and Solr 4.)
• Records can have multiple coordinate sets that are either points or rectangles.
• All coordinates are stored and queried as WENS.
• For display of geographic features in OpenLayers3, coordinates must be ordered as WSEN.
• OpenLayers3 has a bug that causes polygons smaller than .4 decimal degrees to appear/disappear if they are located very far apart. The workaround implemented in this code displays any coordinates sets as a point where N-S < 0.4 decimal degrees and E-W < 0.4 decimal degrees.
• Point coordinates have coordinate values that match W to E and N to S.
• Lat/Lon values are stored to the latlon_coordinate field.
• Decimal degrees values are stored to the bbox_geo field.

To Do List

• Update searches.ini comments to describe new module.
• Decide whether/how to save old Google-based functionality. NOTE: Opted to save it.
• Display search results on map if the number of results is below a certain threshold or look into OpenLayers3 clustering of results so that all results could feasibly be displayed in a clustered fashion.
• Fix ESLint style issues
• Re-write indexing and getter methods to handle a universal set of fields for geographic coordinate and associated data so that either Google or OpenLayers can be used for the Map Tab.
• Convert BeanShell to Java and include in SolrMarc.
• Upgrade SolrMarc in master before merging.
• Internationalization: make pop-up alert help box in Geographic Search utilize standard help system; make sure that all new text strings are included in language files and subject to translation.
• Internationalization of Javascript ("Records at this location:" should use existing "map_results_label" label).

[demiankatz]

Thanks again for sharing this. A few comments:

1.) The Travis build is failing due to some style issues. I've run some automatic cleanup tools, but some problems remain. I can help clean these up if you like, but if you want to go through the exercise yourself, I can leave them for you. (Personally I find code style comformance helpful, but I don't want to force busy-work on you if you have more important things to do).

2.) There were a couple of minor bugs preventing the code from running -- an incorrect namespace on one of the files, and a missing brace in one of the classes that caused a parse error. Presumably these are side effects of copying things from your local customizations into the generic classes... I've corrected them, but there may be more. We'll definitely want to test carefully to be sure we didn't miss anything! (I haven't gotten to the point of running the code yet -- starting out with a higher-level code review).

3.) The OpenLayers files should be kept in js/vendor/ rather than just js/ -- that's how we differentiate between files we wrote and third-party files we incorporated into the project. (This not only helps us manage dependencies better, but it also allows us to avoid getting code style warnings, etc., about code that we don't have control over). I can move these if you don't have time!

4.) We'll want to add a built-in version of your BeanShell script inside SolrMarc before we merge this PR to ensure that BeanShell use is optional. I'll be happy to take care of that. I'm adding a TODO item to the top-level comment so we don't forget about it. Are you happy with the current state of the BeanShell logic? If so, I can port it over now. If you'd prefer to wait until after we've done more testing here, that's fine too -- just let me know!

5.) Right now it looks like the geographic search is hard-coded on in the searchbox.phtml template. I assume we should make this configurable or conditional somehow, since it won't apply to many users.

6.) The map.phtml file has a hard-coded /vufind/themes/bootstrap3/js/ol/ol.css path; this should be converted to a relative path so it works correctly regardless of the location of the VuFind installation.

7.) Another to-do item I've added: we should put something into the upgrade script to warn people that the map tab has been rewritten if they have the "Google" setting turned on. Might also be worth discussing with the group whether there's any reason to keep the old Google functionality in some form (I don't think there is, but it doesn't hurt to ask around to be safe).

Anyway, hopefully that gets the ball rolling. I'll wait to see what you have to say before digging deeper!

[lmgonzales]

Thanks for the feedback Demian. I really appreciate it.

1. I'm happy to clean up the files, but need some help getting started with Travis... I'm not sure how to find which files are failing the build and why. Can you help point me in the right direction?

2. Those issues would be all because of having to copy/paste and hand edit the code in github. Would it be helpful if I sent over a zip file of the working code from our development server? I'll take a look through the files again to catch any mismatches.

3. Okay, good to know. I can fix that and will update the scripts that call them.

4. Yes, the bean shell script works great, so we can add a built-in version to SolrMarc. If you could port it over, that would be really helpful!

5. Ah, yes. I completely forgot to make the geographic search a configurable option in the searches.ini file. I'll have to go back and fix that and then upload the changes.

6. I'll fix that too. Thanks for catching that.

7. Sounds good. I like the idea of asking around as well to find out how many people are using the Google map functionality.

Just an FYI - I've got a lot of projects I'm juggling currently, so it may take me a few weeks to get to all of these items. Thanks again!

[lmgonzales]

P.S. on 1) I figured out Travis - I'll get those files updated appropriately.
Thanks!

[demiankatz]

1. Thanks for the progress on this. If you run into any tricky ones, let me know. After a few years of this, I've finally gotten pretty comfortable with all the tricks for wrapping long lines, etc., etc.

2. If I need a .zip, I'll let you know -- hopefully it will just work when I find time to spin it up and test it, but if it doesn't, I might find that helpful for reference.

3, 5-7) Sounds good!

4. I'll work on the BeanShell conversion in the not-too-distant future.

And, of course, no rush. I'm busy too, so if you take your time, I won't object.

[demiankatz]

@lmgonzales, a couple more minor things:

1.) I notice that the new MapSelection recommendation module is not documented in the list of recommendation modules in searches.ini. Do you mind adding some brief notes there for completeness?

2.) I notice that the indexing routine is currently hard-coded to use 034. Is this a safe assumption? If not, do you have any thoughts on how we should parameterize this? For example, would it make sense to have a field list that specifies north/south/east/west subfields for a series of different fields? The easiest thing is certainly to just leave it as-is for now and add flexibility as needed. However, if you think the need is likely, it would probably be better to address it now, rather than having to rebuild SolrMarc and coordinate another release to add it later.

(I've also added some more checkboxes to the TODO list at the top of the PR to help us keep track of everything).

Thanks!

[lmgonzales]

@demiankatz: Just an update on where things stand with this PR:

1.) Updated searches.ini to add notes to the [MapSelection] module.
2.) Indexing routine hardcoded to use 034.... This is the field that is used by MARC to designate the lat/long coordinate values. 034z is used to attach label/text to the coordinates. (https://www.loc.gov/marc/bibliographic/concise/bd034.html). I think we should keep it as it is and add flexibility in the future if needed.
3.) Moved js/ol/ files to js/vendor/ol and updated MapSelection.phtml and map.phtml files.
4.) Updated searchbox.phtml to remove hardcoded configuration and make it respond to searches.ini configuration for Geographic Search
5.) Moved ol.css to css/vendor/ol/ and updated map.phtml and MapSelection.phtml files.
6.) File clean-up with TravisCI: I ran into a couple of tricky style issues with TravisCI relating to the map_tab.js and map_selection.js files. I've cleaned up all the spacing issues, but I'm not sure I can fix the other errors per the TravisCI suggestions because I think it might break the code. For example it says 'ol' is not defined, but OpenLayers3 uses ol.FunctionName to call a method. Would you mind taking a look at the errors when you have a chance? Thank you!

[demiankatz]

Leila,

I took a bit of time today to look at this; I was planning to help with the ESLint style issues, but I want to get it working before I change anything, and so far, I have not been able to do that. Some notes/questions/comments:

1.) I tried indexing the test records you sent me. The bbox_geo field appears to be getting populated correctly in Solr and recordMap is set to true in config.ini, but I am not seeing a map tab. What else should I be checking?

2.) The Geographic Search link was resulting in a fatal error. I tracked this to a missing line in module.config.php and fixed that, but it's still not working. I think the filter is getting generated incorrectly -- the URL I end up with is: http://my-server/vufind/Search/Results?filter[]=:Intersects(ENVELOPE(bbox_geo)) -- something seems to be missing before the colon. Any ideas?

3.) I notice that the methods you have added to the RecordDriver/SolrMarc.php file are not actually MARC-specific. You should probably move them to RecordDriver/SolrDefault.php so the functionality can be shared with non-MARC data sources in the future.

4.) I've added a TODO checkbox in the top comment to fix the Javascript style issues... once these other things are sorted out, I'll revisit that issue.

Thanks!

[lmgonzales]

@crhallberg, Thanks so much! I had a quick follow up question for you - When I do a git push, should I push the bootstrap3/css/compiled.css and the bootprint3/css/compiled.css, or only just the search.less and map_selection.js files? Thanks!

[demiankatz]

@lmgonzales, you can go ahead and push up the compiled.css files too. It's a little bit redundant, but it's more convenient for everyone that way.

[lmgonzales]

Thanks @demiankatz . When tried to git add the search.less and the boostrap3/css/compiled.css files but am getting the following error:
error: insufficient permission for adding an object to repository database .git/objects error: themes/bootstrap3/css/compiled.css: failed to insert into database error: unable to index file themes/bootstrap3/css/compiled.css fatal: updating files failed
Any ideas? Thanks!

[demiankatz]

Does your .git directory (or some of the files in it) have a different ownership than the user account that you are logged in under when attempting to commit?

[lmgonzales]

No. Their group and owner are the same as my user account, and permissions are set the same as the bootprint3/css/compiled.css that I was able to add.

[demiankatz]

Very weird. Does sudo help?

[lmgonzales]

Let me see if I can make the changes manually via the github web interface for those two files. and then git push the other two.

[lmgonzales]

@demiankatz,
I've completed the css changes, and also updated code to use the map_results_label instead of the hard-coded popup title. Please let me know if you see anything else that needs to be fixed/updated/etc.
Thanks!

[demiankatz]

@lmgonzales, thanks a lot! I'll give this a closer look on Monday. If nothing new comes up in the way of problems, I think we'll be able to merge it.

[demiankatz]

@lmgonzales, I've just put on some finishing touches: I've turned off MapSelection by default, and I've updated/expanded/corrected some of the [MapSelection] comments in searches.ini. Once @crhallberg has time to do his review, I think we can merge this.

Just one minor question in the meantime: the searches.ini comment for default_coordinates addresses coordinate order, but it doesn't describe ranges of valid values. Is it worth adding another sentence or two to elaborate on what coordinate system is being used and how to determine values?

[lmgonzales]

@demiankatz , Thanks for making the finishing touches to the code. I've added a few sentences to searches.ini to elaborate on the coordinate type and also the range of valid values, as well as tips on what to do if the dataset is geographically distributed rather than concentrated in one area. Essentially, we shouldn't encourage users to select the full map extent, otherwise the search could be slow if they have a large database.

[demiankatz]

@lmgonzales, wonderful, thank you! I expect we will merge this tomorrow, barring any last-minute problems.

[lmgonzales]

@demiankatz, That's great news! Here's hoping for no last-minute problems!

[demiankatz]

No last-minute problems -- merged, with thumbs-up from @crhallberg.

[lmgonzales]

Great to hear! Thanks @demiankatz and @crhallberg!!