Addressed issue #94 - Githubissues

Islandora-Labs / islandora_solution_pack_oralhistories

Adds all required Fedora objects to allow users to ingest and retrieve Oral Histories (video/audio) files through the Islandora interface

GNU General Public License v3.0

13 stars 23 forks source link

Addressed issue #94 #95

Closed McFateM closed 6 years ago

McFateM commented 7 years ago

This PR addresses Issue #94 by producing a warning via drupal_set_message() to indicate that the oral history object returned no Solr query results. The generated message will look something like this:

screen shot 2017-06-27 at 2 31 05 pm

Testing:

Remove the Solr document for one existing OH object (I used the admin interface at http://my.repository.address:8080/fedoragsearch/rest to do this)
Attempt to open/view the target OH object. The warning should appear.
Restore the Solr document for the target OH object (re-index the object in Solr). The warning should no longer appear.

@MarcusBarnes @kimpham54

MarcusBarnes commented 7 years ago

@McFateM Thanks for this suggestion. Would you take look at your Drupal Watchdog log for messages that would indicate that the Solr query was unable to retrieve the transcript? The pattern we have been using is to log events that are relevant to admins and developers to Watchdog rather than to the regular user UI.

McFateM commented 7 years ago

Hi Marcus. Sorry for the delayed reply…been having network issues here.

I noticed the Watchdog code in the try…catch statement and went looking to see what was reported, but I found nothing. I presume that’s because in my case the try is valid, it is a valid query, but the response if “empty” since the object hasn’t been indexed in Solr yet. So I thought about adding another Watchdog entry but since the existing one wasn’t very helpful I was reluctant to do so.

If you would prefer to reform the Warning as just a Watchdog entry, or make any changes, I’m OK with that, for consistency.

-Mark M.

MarcusBarnes commented 7 years ago

@McFateM I propose improving the watchdog log message to provide the additional information you included in your pull request. Thanks again for your thoughts.

MarcusBarnes commented 6 years ago

Suggestions on how I could have avoided including all the other commits around 218942e005fc4c2af8eb291b659e40cca1183b02? Once Travis CI is done, this should still be able to be merged successfully, but clearly there could have been a better approach when I followed the command line instructions for testing the pull-request locally, committed my changes, then pushed back.

MarcusBarnes commented 6 years ago

This is failing on PHP 5.3.3 due to changes to Travis CI - this has been encountered by others in other projects. I'll confer with the other project maintainers. We may by-pass the PHP 5.3.3 Travis check for now until we an adequate check in place.

MarcusBarnes commented 6 years ago

@McFateM Since I've appeared to have messed up the history in this pull-request when attempting to push my modifications to your suggestion, I'm going to close this pull-request and reopen another with the change. I'll be sure to cite your original approach. Thanks again!