Use Solr for newspaper issue listing #158

whikloj · 2017-12-01T18:51:58Z

JIRA Ticket: https://jira.duraspace.org/browse/ISLANDORA-2126

What does this Pull Request do?

Add a option to generate the newspaper issue list from Solr instead of the triplestore.

What's new?

New variables, split the islandora_newspaper_get_issues() function to call either Solr or the resource index.

How should this be tested?

You'll need a newspaper with several issues.

Download this PR and install it.

Go to the admin page, choose Use Solr and edit the 3 new fields as necessary. Save the configuration.

Go to the newspaper and see exactly the same display as before.

Additional Notes:

Any additional information that you think would be helpful when reviewing this PR.

Example:

Does this change require documentation to be updated? Documentation included
Does this change add any new dependencies? no
Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? no
Could this change impact execution of existing code? no

Interested parties

@bgilling @mjordan @rosiel

DiegoPino

Mostly feedback on minor stuff

DiegoPino · 2017-12-01T18:57:24Z

includes/admin.form.inc

+      '#title' => t('Use Solr for Newspaper display'),
+      '#disabled' => (!module_exists('islandora_solr')),
+      '#description' => t('Use Solr to generate lists of issues for a newspaper object.'),
+      '#default_value' => (module_exists('islandora_solr') ? variable_get('islandora_newspaper_use_solr', FALSE) :


Do we need to check if the module exists? Makes sense of course but we have existing settings (isMemberOf_uri_ms) that don't do that/depend on the existence (like 42 to 51). What about fixing all and making module exist part of the Solr options subform validation/ or even better display?

We don't officially have a dependency on Solr, so I figured it's a good idea to check for it. I'm good either way.

DiegoPino · 2017-12-01T18:59:44Z

includes/admin.form.inc

+        '#title' => t('Issued date field'),
+        '#description' => t('Solr field that contains the date issued of the newspaper issue.'),
+        '#size' => 100,
+        '#default_value' => variable_get('islandora_newspaper_solr_date_field', 'RELS_EXT_dateIssued_literal_ms'),


Is the new logic sorting against a date? In that case, the default could be better _s or even better _dt to allow actually an order by date_issued in the query itself

No, this does not do that. It would require more work (which I considered) to check if the field was a date and then sort in Solr. But this returns just returns the results which are grouped and sorted in PHP.

@whikloj don't want to be insistent, but in the rare case that you actually have an _ms field with multiple values, will the display/after logic )(not yours, what is already in place) work out correctly? What happens if you get two dates? This is just plain curiosity now

Ok, so for multi-valued fields I am just grabbing the first. With a date issued it seems like you shouldn't have a bunch of them. But I'm open to suggestions.

DiegoPino · 2017-12-01T19:00:32Z

includes/admin.form.inc

+        '#title' => t('Sequence field'),
+        '#description' => t('Solr field that contains the sequence number of the issues in a newspaper.'),
+        '#size' => 100,
+        '#default_value' => variable_get('islandora_newspaper_solr_sequence_field', 'RELS_EXT_isSequenceNumber_literal_ms'),


A bit of the same as previous, maybe better to default to something that can be sorted on via Solr (single valued)

Same answer around sorting, depends on non-standard Solr config. Could be a new feature to add in later.

DiegoPino · 2017-12-01T19:01:19Z

includes/admin.form.inc

+ * Implements hook_FORM_ID_validate().
+ */
+function islandora_newspaper_admin_settings_form_validate(array $form, array &$form_state) {
+  $error = array();


Prob this spot is good for checking if solr module exist? (comment only, not requesting a change in this one)

DiegoPino · 2017-12-01T19:03:16Z

includes/utilities.inc

+    catch (Exception $e) {
+      // Use the current time as a place holder.
+      $issued = new DateTime();
+      $msg = 'Failed to get issued date from SPARQL query for @pid';


Should this say "From the Solr query?"

DiegoPino · 2017-12-01T19:08:05Z

includes/utilities.inc

+  );
+
+  do {
+    $start += 1;


Regarding the default dates/sequence, (_the _ms issue I mentioned comments before), could a sort of be added to the Solr query? Not needed?

DiegoPino · 2017-12-01T19:13:02Z

includes/utilities.inc

+      drupal_set_message(check_plain(t('Error searching Solr index')) . ' ' . $error->getMessage(), 'error');
+    }
+
+  } while ($count > ($rows * $start + $rows) && !isset($error));


should $count be initialized to something?

Line 222 above, basically in the case that you have more than 10,000 issues I wanted to start looping to grab them. So the count tells me the total number but only after we have done a query. I could initialize it before the loop if you think that is better. I added the !isset($error) to halt the loop if you get an error from Solr.

yeah, the !isset($error) is a good one. I was just afraid that at runtime, PHP 7.x could throw some warnings because $count is undefined. I have no strong opinion really but want to be sure the logic does not allow infinite looping (i'm scared of "while" statements always). Seems like the logic won't allow that. good

Github is doing something weird. I added a $error = 0; to line 193, which you can see on the branch but in the PR it appears like maybe it didn't fully update? Not sure
https://github.com/uml-digitalinitiatives/islandora_solution_pack_newspaper/blob/7.x-ISLANDORA-2126/includes/utilities.inc#L193

@whikloj weird. I have seen some caching weirdness in GitHub lately. Can you force a new hash via an ammend or a rebase? 😭

@whikloj still don't see $error = 0; Should i wait for that?

DiegoPino · 2017-12-01T19:42:01Z

includes/utilities.inc

@@ -80,6 +77,41 @@ function islandora_newspaper_get_newspaper($object) {
 *     - issued: A DateTime object repersenting the date the issue was released.


Not your fault, was there from forever but could be good chance to fix "repersenting"

DiegoPino · 2017-12-01T19:52:06Z

includes/admin.form.inc

@@ -139,7 +141,7 @@ function islandora_newspaper_admin_settings_form_validate(array $form, array &$f
  }
  if (count($error) > 0) {
    foreach ($error as $field => $message) {
-      form_set_error($field, $message);
+      form_set_error($field, check_plain($message));


DiegoPino · 2017-12-01T19:52:45Z

@whikloj looks good. Will wait for travis.

DiegoPino · 2017-12-01T20:08:46Z

/remind me to approve once @whikloj rehashes and then merge in 24 hours

DiegoPino · 2017-12-01T20:10:37Z

@dannylamb should something had happened with /remind thing? Do we need to have the Bot actually running?

whikloj · 2017-12-01T20:39:52Z

Best git commit message ever!

Sorry @DiegoPino I forgot to make the OR explicit

Don't alter the results

DiegoPino

Jared. Does removing base filters still allows for namespace and xacml based exclusions?

whikloj · 2017-12-04T15:49:50Z

@DiegoPino So namespaces and XACML should be ok. The whole reason to use the IslandoraSolrQueryProcessor is those exclusions.

The issue I hit was I have a base filter which removes newspaper issues from search results, because most people what to see the actual page.

But that filter means that I can't generate a listing of all newspaper issues via Solr unless I remove it.

Here is the admin form for the base filters.
https://github.com/Islandora/islandora_solr_search/blob/7.x/includes/admin.inc#L613-L620

and that we only remove those specific filters from the query.
https://github.com/Islandora/islandora_solr_search/blob/7.x/includes/utilities.inc#L651-L657

I'm not sure what others use that filter for and if this would produce unwanted consequences for them.

DiegoPino · 2017-12-04T15:59:18Z

@whikloj totally. We will actually need in the near future to separate internal query processor v/s final user display one. Thanks for clarifying this. Now that DCS are Ok, I will test this again during the day. Give me 24 hours before merging.

whikloj · 2017-12-04T16:08:23Z

If this seems dangerous, then we can close it. Most of this code is already in my islandora_custom_solr module to allow me to do my queries in Solr.

I have a follow-up that uses Solr date fields to add a range filter to retrieve only one years worth of newspapers at a time. It then uses Ajax (prompted by @bgilling's work here) to load any other year dynamically.
https://github.com/uml-digitalinitiatives/islandora_solution_pack_newspaper/compare/7.x-ISLANDORA-2126...uml-digitalinitiatives:7.x-ISLANDORA-2116-2?expand=1

I can also build this into islandora_custom_solr instead of core if there are concerns are my use of islandora_solr_remove_base_filters().

DiegoPino · 2017-12-04T18:04:26Z

@whikloj i actually don't have any concerns with islandora_solr_remove_base_filters(). But I guess folks could? Any way we could make some exclusions without touching all the filters? Maybe just too much work, i know. @Islandora/7-x-1-x-committers any concerns?

whikloj · 2017-12-04T18:55:11Z

@DiegoPino I guess we could add a toggle, I'm not sure how to clearly explain it.

[x] Remove base filters
This option removes your configured Solr base filters from the queries. If you want your filters to be applied even though they could affect which newspaper issue objects are returned in the list, uncheck this option.

or something....better?

DiegoPino · 2017-12-04T20:26:57Z

I like the toggle! El El lun, 4 de dic. de 2017 a las 13:55, Jared Whiklo < [email protected]> escribió:

@DiegoPino <https://github.com/diegopino> I guess we could add a toggle, I'm not sure how to clearly explain it. [x] Remove base filters This option removes your configured Solr base filters from the queries. If you want your filters to be applied even though they could affect which newspaper issue objects are returned in the list, uncheck this option. or something....better? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#158 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGn859dAeVOaSpLcrZuXSADted2CeRZ4ks5s9EAQgaJpZM4QywkJ> .

-- Diego Pino Navarro Digital Repositories Developer Metropolitan New York Library Council (METRO)

whikloj · 2017-12-11T17:12:13Z

Ok, toggle is added.

So to test the toggle, if you go to Admin -> Islandora -> Solr Index -> Solr Settings -> Query Defaults tab

In the Solr base filter field add -RELS_EXT_hasModel_uri_ms:"info:fedora/islandora:newspaperIssueCModel"

Then by default this PR works, but if you un-check the new Remove base Solr filters toggle, then your base filters are NOT removed and you should get 0 issues displayed for your test newspaper.

DiegoPino · 2017-12-11T20:29:59Z

@whikloj hi, looks almost ready, great. Could you please just fix DCS thing at https://travis-ci.org/Islandora/islandora_solution_pack_newspaper/jobs/314886467#L917 will later approve and go the usual merge workflow if nobody opposes. Thanks

DiegoPino · 2018-03-19T17:37:16Z

@whiklojI approved the changes here but DCS issue is still unsolved. Please let me know if you want me to pull against your branch to ease on that.

whikloj added 3 commits November 30, 2017 16:44

Add a solr query for issues list

01c7a04

Update README

c184f1c

Lots of missing variables to uninstall

feadea6

DiegoPino suggested changes Dec 1, 2017

View reviewed changes

DiegoPino reviewed Dec 1, 2017

View reviewed changes

Code review and travis issues

5cc2920

DiegoPino reviewed Dec 1, 2017

View reviewed changes

whikloj added 2 commits December 1, 2017 14:33

PHP 5.3git add includes/utilities.incgit add includes/utilities.inc

3b7ec90

Make OR explicit

32d9eb1

Remove base filters to ensure we get all results

f7db27b

Don't alter the results

DiegoPino reviewed Dec 4, 2017

View reviewed changes

Need to override limit or you get what is set in the config.

5008587

Code review

f3c174b

Include toggle to disable removing solr base filters

cae4f2e

DiegoPino approved these changes Mar 19, 2018

View reviewed changes

Add last comma

eb75190

DiegoPino merged commit bd5ba36 into Islandora:7.x Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Solr for newspaper issue listing #158

Use Solr for newspaper issue listing #158

whikloj commented Dec 1, 2017

DiegoPino left a comment

DiegoPino Dec 1, 2017

whikloj Dec 1, 2017

DiegoPino Dec 1, 2017

whikloj Dec 1, 2017

DiegoPino Dec 1, 2017

whikloj Dec 4, 2017

DiegoPino Dec 1, 2017

whikloj Dec 1, 2017

DiegoPino Dec 1, 2017

DiegoPino Dec 1, 2017

DiegoPino Dec 1, 2017

DiegoPino Dec 1, 2017

whikloj Dec 1, 2017

DiegoPino Dec 1, 2017

whikloj Dec 1, 2017

DiegoPino Dec 1, 2017

DiegoPino Dec 11, 2017

DiegoPino Dec 1, 2017

DiegoPino Dec 1, 2017

DiegoPino commented Dec 1, 2017

DiegoPino commented Dec 1, 2017

DiegoPino commented Dec 1, 2017 •

edited

Loading

whikloj commented Dec 1, 2017

DiegoPino left a comment

whikloj commented Dec 4, 2017

DiegoPino commented Dec 4, 2017

whikloj commented Dec 4, 2017

DiegoPino commented Dec 4, 2017

whikloj commented Dec 4, 2017

DiegoPino commented Dec 4, 2017 via email

whikloj commented Dec 11, 2017

DiegoPino commented Dec 11, 2017

DiegoPino commented Mar 19, 2018

		@@ -80,6 +77,41 @@ function islandora_newspaper_get_newspaper($object) {
		* - issued: A DateTime object repersenting the date the issue was released.

Use Solr for newspaper issue listing #158

Use Solr for newspaper issue listing #158

Conversation

whikloj commented Dec 1, 2017

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

DiegoPino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoPino commented Dec 1, 2017

DiegoPino commented Dec 1, 2017

DiegoPino commented Dec 1, 2017 • edited Loading

whikloj commented Dec 1, 2017

DiegoPino left a comment

Choose a reason for hiding this comment

whikloj commented Dec 4, 2017

DiegoPino commented Dec 4, 2017

whikloj commented Dec 4, 2017

DiegoPino commented Dec 4, 2017

whikloj commented Dec 4, 2017

DiegoPino commented Dec 4, 2017 via email

whikloj commented Dec 11, 2017

DiegoPino commented Dec 11, 2017

DiegoPino commented Mar 19, 2018

DiegoPino commented Dec 1, 2017 •

edited

Loading