This is a replacement pull request for #323 #332
Conversation
|
Can you try restarting those two failing builds in Travis? |
|
@bondjimbond I hit the refresh button on those. Fingers crossed 🤞 |
|
@dnwk Can you provide a bit more detail on how to replicate the actual problem that this PR solves? e.g. some sample data (both for MODS and a specific setting for the Custom XPATH field) that we can use to verify the issue and see what's different after checking out this PR? |
I am fixing a bug here. There is a custom XPATH in admin settings that let you set the XPath for Google Scholar tag. However, the actual code that generate the Google tag completely ignored whatever you set in that admin panel and just using the default. That's true for how author tag was generated. |
|
OK, so the problem I'm encountering here...
Using the default setting for the Authors field, here's the output to compare: 7.x branch: This PR: |
|
@bondjimbond In the code for author, I am looking for mods:namePart under the author path. I guess you could say it should blindly take whatever is in the XPath user designated. |
|
@bondjimbond Could you also post the MODS? |
|
Sure, here's the MODS... |
includes/google_scholar.inc
Outdated
| @@ -171,7 +178,7 @@ function islandora_scholar_create_meta_tags($object) { | |||
|
|
|||
| $online_date = $mods_xml->xpath(variable_get('islandora_scholar_xpaths_online_date', '//mods:recordInfo/mods:recordCreationDate')); | |||
| if ($online_date) { | |||
| $date_string = islandora_scholar_parse_date_foryear($online_date); | |||
| $date_string = islandora_parse_date_foryear($online_date); | |||
There was a problem hiding this comment.
This change breaks the page load for me; the function islandora_parse_date_foryear does not exist in any of the Islandora modules. islandora_scholar_parse_date_foryear does exist later on in this same file though at https://github.com/Islandora/islandora_scholar/blob/7.x/includes/google_scholar.inc#L210-L229.
I'm assuming this is just a typo, but this needs to be changed back to the way it was originally before this PR can be accepted.
includes/google_scholar.inc
Outdated
| @@ -44,36 +43,46 @@ function islandora_scholar_create_meta_tags($object) { | |||
| else { | |||
| return FALSE; | |||
| } | |||
| foreach ($mods_xml->xpath('mods:name') as $name_xml) { | |||
| foreach ($mods_xml->xpath(variable_get('islandora_scholar_xpaths_authors_xpath', 'mods:name')) as $name_xml) { | |||
There was a problem hiding this comment.
This is good, we should definitely not be using the hardcoded xpath if a configurable one exists.
|
@dnwk Okay! I've spent a long time looking at this and messing with it, and I've updated the code review several times as my understanding evolved, up to the point of dismissing the code review entirely so we can just talk about it. The first thing that needs to be fixed is the bad function name, but I see that you saw that and will put in a fix. The second thing I realized is that by adding the call to the variable for custom authors xpath and removing the pivot on The problem that remains is that most people who were using the old code that assumed Last but not least, when I upload a MODS record with I still don't see a Moving forward, after fixing the function name I'd say the next step is to make sure that the |
|
Also just because I didn't say it before, THANK YOU for sticking with this PR and bearing with folks as they try to grapple with it. There's an intense amount of cognitive load going on here to even understand the problem or how the solution works, but this is a good idea and definitely addresses a bug that ought to be corrected. ❤️ |
@bryjbrown In my XPath, my XPath stopped at //mods:mods[1]/mods:name[@displayLabel= "Creator(s)"] So that my code will searching for the namePart underneath it. I am sure there is a way to write XPath to point to mods:name where it has roleTerm=author. So, probably in the default "mods:name" should be rewrite to somehow with an author in roleTerm. Let me research how I could write a XPath that default to mods:name where it has children of role\roleTerm="author". If you have an idea, let me know. Thanks |
|
@dnwk I think thats what the is doing, I'm no xpath wizard so I can't verify one of the top of my head, but if you are trying specifically to get the and see if that woks as a default value. |
|
@bryjbrown I am using some online XPath testing tool and settled on /mods/name/role[roleTerm = "author"]/.. So that it will select the entire mods:name node for further processing. |
… and change mods:name default xpath to "//mods:mods[1]/mods:name/mods:role[mods:roleTerm = "author"]/.." for legacy usage that relied on "author" role term
|
@bryjbrown update my code to fix the typo and updated default mods:name xpath. I have no way to test if the new xpath would fit into previous xpath assumption. Could you test it? |
|
@dnwk I see your new commit and it looks good at first glance; putting this on my to do list to review sometime this week hopefully. Thanks for following up on this! |
| @@ -171,7 +179,7 @@ function islandora_scholar_create_meta_tags($object) { | |||
|
|
|||
| $online_date = $mods_xml->xpath(variable_get('islandora_scholar_xpaths_online_date', '//mods:recordInfo/mods:recordCreationDate')); | |||
| if ($online_date) { | |||
| $date_string = islandora_scholar_parse_date_foryear($online_date); | |||
| $date_string = islandora_scholar_date_foryear($online_date); | |||
There was a problem hiding this comment.
We originally had islandora_scholar_parse_date_foryear which was correct but the original commits changed this to islandora_parse_date_foryear leaving out the _scholar_ bit. Now it appears that you have added _scholar_ but in the process left out the _parse_ part. This needs to be islandora_scholar_parse_date_foryear, anything else causes the object page to fail to load. Please test a page load for the object before committing to verify that the object page loads.
|
@dnwk The new default xpath looks like its heading in the right direction, but still not working for me. When I switch from the main 7.x branch to this PR with the custom xpaths turned off, the the Furthermore, it looks like in adding the Since I'm rapidly approaching the limits of my xpath knowledge, I'm requesting @DonRichards take a look at this. He wrote the custom xpath bits of Scholar so he should have a better idea of what's going on here. |
|
Hi @bryjbrown @dnwk and @DonRichards - Don and I were talking about this on our local Slack. While an XPath expression like
would possibly make more sense. I'm not sure exactly what I would need to do locally to fully test this PR and help with any XPath issues, so any notes along those lines would be welcome if you'd like help testing the XPath expressions here. Would the islandora labs vagrant be sufficient? Lastly, @dnwk in your initial comment, if you have local MODS that uses In any case, I hope everyone doesn't mind an additional voice in the conversation. Apologies if I'm disrupting a mostly-finished PR. Best! Edit: I'm asleep at the wheel this morning. ☕ |
|
@CanOfBees I have been using the Islandora Vagrant VM to test, and using this MODS record: <?xml version="1.0" encoding="UTF-8"?>
<mods:mods xmlns="http://www.loc.gov/mods/v3" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:flvc="info:flvc/manifest/v1" xmlns:mods="http://www.loc.gov/mods/v3" xmlns:dcterms="http://purl.org/dc/terms/" xsi:schemaLocation="http://www.loc.gov/standards/mods/v3/mods-3-4.xsd" version="3.4">
<titleInfo lang="eng">
<title>Scholar Test Document</title>
</titleInfo>
<name type="personal" authority="local">
<namePart type="given">Arthur</namePart>
<namePart type="family">Author</namePart>
<role>
<roleTerm authority="rda" type="text">author</roleTerm>
<roleTerm authority="marcrelator" type="code">aut</roleTerm>
</role>
</name>
<name type="personal" authority="local">
<namePart type="given">Edward</namePart>
<namePart type="family">Editor</namePart>
<role>
<roleTerm authority="rda" type="text">editor</roleTerm>
<roleTerm authority="marcrelator" type="code">ed</roleTerm>
</role>
</name>
<abstract>Test document for testing capture of authors.</abstract>
<typeOfResource>text</typeOfResource>
<genre authority="rdacontent">text</genre>
<language>
<languageTerm type="text">English</languageTerm>
<languageTerm type="code" authority="iso639-2b">eng</languageTerm>
</language>
<physicalDescription>
<form authority="rdamedia" type="RDA media terms">computer</form>
<form authority="rdacarrier" type="RDA carrier terms">online resource</form>
<extent>1 online resource</extent>
<digitalOrigin>born digital</digitalOrigin>
</physicalDescription>
</mods:mods>It has a name with roleTerm=author and a name with roleTerm=editor, so you can test that the default works and should pick up "Arthur Author", but you should also be able to turn on the custom xpath queries and set the "author" selector xpath to point to roleTerm=editor and grab "Edward Editor" instead. In my experience, the master 7.x branch grabs "Arthur Author" by default, but when custom xpaths are turned on and the author xpath is set to target roleTerm=editor instead of roleTerm=author, it STILL grabs "Arthur Author" as the author when it should be grabbing "Edward Editor". With the PR branch, it fails to grab anything whether custom xpaths are turned on or not. |
|
Re-reviewed this PR as a result of today's 7x Committers Call. I think this PR is still valid and addresses an important bug that should get taken care of before the next release, so we do need to get this wrapped up and merged. The current problem that needs to be solved before this PR can be merged is that we need the
It seems like the 7.x branch of Islandora Scholar currently works for 1, but not 2 or 3. I'm unsure if this PR gets 2 and 3 to work, but it definitely does not work with 1 currently. We need to get all 3 working as expected to move forward. Since we haven't heard from @dnwk in a bit, I'd like to ask @DonRichards (and perhaps @CanOfBees if possible) to move forward with this since @DonRichards built the custom XPath machinery and has the most insight into how it works. I believe the core of the issue revolves around https://github.com/dnwk/islandora_scholar/blob/7.x-Kun-GoogleScholarDev/includes/google_scholar.inc#L47 which is using the value set for the |
This is a replacement pull request for #323
This is a pull request to make sure Google Scholar submodule will properly use the custom XPath settings in admin/islandora/solution_pack_config/scholar/xpaths for generating Google Scholar metatags, especially for authors.
In admin/islandora/solution_pack_config/scholar/xpaths, there are ways to set custom XPath for author information, including XPath for First Name, Last Name and one other fields just labeled as Authors. Currently in the file islandora_google_scholar.module, in line 43, it would just use “mods:name” as XPath and completely ignored what is set in the custom xpath settings. Furthermore, in line 50-54, it requires mods:role/mods:roleTerm and the roleTerm must be “author”. If roleTerms does not exist in MODS or the role is not author, it will not generate any information for authors. Since Google Scholar tags requires author information, it will not generate any Google Scholar tags if MODS does not match the hard-coded format exactly. Here is our sample MODS that current module will not be able to generate google scholar tags:
There are situations that “name” in MODS could be editors or advisors. However, since in the admin panel for custom xpath stated this is the xpaths for author, the custom xpaths should be accepted as author without roleTerm restriction. I also made accommodations in the code for middle name since it will have a namePart type given.
All the other required fields in the google scholar module works as intended and is taking custom xpath settings from the admin panel. I have tested my code in our islandora staging environment and it is generating google scholar meta tags in html header.
What's new?
How should this be tested?