Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Google Scholar submodule modification for properly taking custom xpaths for authors #323

Open
wants to merge 97 commits into
base: 7.x
Choose a base branch
from

Conversation

dnwk
Copy link

@dnwk dnwk commented May 22, 2019

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:

<name displayLabel="Creator(s)" type="personal"> <namePart type="given">Brian</namePart> <namePart type="given">M*****</namePart> <namePart type="family">G****</namePart> <displayForm>Brian M***** G*****</displayForm> <role> <roleTerm>Creator(s)</roleTerm> </role> </name>

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?

  1. variable_get('islandora_scholar_xpaths_authors_xpath') to get custom authors xpath while keeping mods:name as default.
  2. Ignore roleTerm in MODS since the custom xpath settings specific says it is for authors.
  3. Take mods:displayForm as a backup source for name if namePart type does not match “family” nor “given”.
  4. It will accommodate more than one “given” name as middle name is marked as “given” in MODS.

How should this be tested?

  1. Find an object in Islandora that’s covered by Islandora Scholar module.
  2. In admin/islandora/solution_pack_config/scholar/xpaths, set custom xpaths for authors.
  3. Visit the object page and check HTML header to make sure meta name="citation_author" exists and is showing the correct information.

Copy link
Contributor

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull seems to be ok and deals with the bug, but needs some heavy cleanup (Drupal Coding Standards). A pass through phpcs or any editor that can deal with our format should suffice. I also made a question about values there, since this pull adds as last option displayForm but does not check if there is an actual value or not. I also wonder here what happens if you happen to have more than one displayForm? (seems to me its valid MODS still)

@manez
Copy link
Member

manez commented May 23, 2019

Thank you very much for this fix @dnwk! In order to accept new code into Islandora, we require that contributors complete a Contributor License Agreement (if it's all your own work) or Corporate Contributor License Agreement (if it's work done on behalf of your employer), to nail everything down as open source. Details and links to the different CLA options are here

@manez
Copy link
Member

manez commented May 23, 2019

CLA now on file! This is good to go on that front. Thank you @dnwk

@bondjimbond
Copy link
Contributor

Github says that this whole thing is written on one line... That can't be right.

@DiegoPino
Copy link
Contributor

@dnwk you could try with Atom (free) and has good Drupal support. Seems like most of your issues are "Windows" induced.

@DonRichards
Copy link
Member

DonRichards commented May 23, 2019

I agree with @DiegoPino on your issue. This might be a character return problem with your editor or with your git config. This will save you a lot a headaches for Windows environment. I wouldn't suggest it for other operating systems.

$ git config --global core.autocrlf true

More info on core.autocrlf

@dnwk
Copy link
Author

dnwk commented May 23, 2019

@DonRichards Thanks. I just got Atom installed. It works like a charm.

Copy link
Author

@dnwk dnwk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Style clean up to match Drupal standard
  2. name_DisplayForm is now taking precedent over no_type and is checking if content exist.
  3. adding (string) to taking only one name_DisplayForm.

@dnwk
Copy link
Author

dnwk commented Jul 24, 2019

I just ran the rebase with 7.x again. google_scholar.inc is the only file modified by me now.

…for the tag, it will actually using what's configured in the admin menu.
@DonRichards
Copy link
Member

@DiegoPino Did @dnwk changes address all your request for changes?

@@ -49,6 +49,48 @@ Additional XPath Configurations:

![ISLANDORA SCHOLAR XPATH CONFIGURATION](https://user-images.githubusercontent.com/11573234/48782673-b3362400-ecac-11e8-869c-3928c43df253.PNG)

xxx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a mess. Is this the intended output?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure which one are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry for the long delay on answering you -- I didn't get notified that you replied!)

The README file has a lot of weird text in it:

xxx

location of replacement file
https://raw.githubusercontent.com/cdeaneGit/islandora_scholar/ISLANDORA-2338/XPATH_config2_full.JPG


This is where the replacement goes.

zzz

Also, if I look at your README in your fork: https://github.com/dnwk/islandora_scholar

The image links are broken.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it by pulling the file from upstream

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The messy stuff is still there.

@DonRichards
Copy link
Member

It looks like you still need to rebase properly.

@whikloj
Copy link
Member

whikloj commented Apr 14, 2020

@dnwk I tried to rebase your branch for you, but I didn't fully understand what you were changing.

So instead I tried to cherry-pick the last 4 commits except the merges.

Does this look correct?
7.x...uml-digitalinitiatives:try-rebase

@dnwk
Copy link
Author

dnwk commented Apr 14, 2020

@dnwk I tried to rebase your branch for you, but I didn't fully understand what you were changing.

So instead I tried to cherry-pick the last 4 commits except the merges.

Does this look correct?
7.x...uml-digitalinitiatives:try-rebase

I created a new pull request #332 so that it would be easier to see what I have changed.

@DonRichards
Copy link
Member

@dnwk That looks correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.