Sitemap & Google Scholar #379

pgwillia · 2018-01-15T21:21:42Z

Replaces #340 Closes #9

Get the modified date indexed into Solr
Item visibility (filter out private items)
Add a static robots.txt file?
Other requirements from Google Scholar requirements: Google Scholar optimization #234
Change SitemapPolicy to simply inherit from ApplicationPolicy.
Add an application specific property on fileset to include hash, file size, and mimetype information.
Theses.
splitting into sitemapindex and sitemap

murny · 2018-01-15T22:33:05Z

app/views/items/show.html.erb

@@ -1,5 +1,7 @@
 <% page_title(@item.title) %>

+<%= render partial: 'google_scholar_metadata' %>


These meta tags will be in the <body>. Should probably inject them into the <head> of the page instead?

Requires some additional work to make this happen. Look into content_for and yield if you need help figuring out how to do this.

mbarnett · 2018-01-18T20:02:32Z

app/controllers/sitemap_controller.rb

+
+  def communities
+    @communities = Community.all
+    raise 'sitemap should contain less than 50,000 targets' if @communities.count > 50_000


I don't think we'll want to start serving Google a 500 page if/when any of these creeps above 50,000 without us noticing. Maybe replace raise with logger.warn? (actually even better would be to push a warning to Rollbar, but we don't have that set up yet. Maybe add a TODO: about that as well?)

mbarnett · 2018-01-18T20:37:34Z

app/models/concerns/item_properties.rb

@@ -134,6 +148,10 @@ def display_attribute_names
    def valid_visibilities
      super + [VISIBILITY_EMBARGO]
    end
+
+    def public
+      Item.where(visibility: JupiterCore::VISIBILITY_PUBLIC)


Since this concern is shared with Thesis, I don't think this quite works (Thesis.public will return public Items, instead). self.where(visibility: JupiterCore::VISIBILITY_PUBLIC) should do the appropriate thing in both classes.

I'd also love to see this get exercised in the test suite. Probably just add another assertion to these two existing test cases: https://github.com/ualbertalib/jupiter/blob/master/test/models/thesis_test.rb#L5 and https://github.com/ualbertalib/jupiter/blob/master/test/models/item_test.rb#L5

mbarnett · 2018-01-18T20:41:01Z

app/views/items/_google_scholar_metadata.html.erb

+  <meta name="citation_issue" content=""/>
+  <meta name="citation_firstpage" content=""/>
+  <meta name="citation_lastpage" content=""/>
+-->


Do we actually want to output this whole set of comments into the HTML, or do we want this comment not to render at all? (Use <%# %> is the latter).

mbarnett · 2018-01-18T20:43:12Z

app/models/concerns/item_properties.rb

+"#{unlocked_fileset.original_file.checksum.value}\" \
+length=\"#{unlocked_fileset.original_file.size}\" \
+type=\"#{unlocked_fileset.original_file.mime_type}\"\
+/>"


Rather than have to escape all of these newlines, this is probably a great use-case for a heredoc: https://infinum.co/the-capsized-eight/multiline-strings-ruby-2-3-0-the-squiggly-heredoc

This is actually more like a run-on sentence situation. There should be no newlines or extra spaces/tabs between the values. My understanding is that heredoc would maintain newlines. Is there a solution to make something like that more readable?

mbarnett · 2018-01-18T20:52:48Z

One other thing I'd love to see would be an added system test exercising the sitemap. But this looks really good other than the small things noted above.

mbarnett · 2018-01-22T18:03:17Z

public/robots.txt

@@ -1 +1,2 @@
 # See http://www.robotstxt.org/robotstxt.html for documentation on how to use the robots.txt file
+Sitemap: https://era.library.ualberta.ca/sitemap.xml


Coming out of a discussion with Kenton this morning, it sounds like it would be useful to make this dynamic and serve

User-agent: * Disallow: /

based on a config key in the Rails environment config files, so that we can tell crawlers to go away during the beta launch phase.

(Basically, set config.some_key_name in the various env files, and then do if Rails.configuration.some_key_name == true here

houndci-bot · 2018-01-24T22:51:17Z

test/controllers/sitemap_controller_test.rb

+    should 'show url, last modified date, and content attributes' do
+      #visit items_sitemap_path
+      #assert_selector 'url', count: 4
+      #assert_selector 'loc', text: 'sitemap-items.xml'


Layout/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)

houndci-bot · 2018-01-24T22:51:17Z

test/controllers/sitemap_controller_test.rb

+    end
+    should 'show url, last modified date, and content attributes' do
+      #visit items_sitemap_path
+      #assert_selector 'url', count: 4


Layout/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)

houndci-bot · 2018-01-24T22:51:17Z

test/controllers/sitemap_controller_test.rb

+      assert_empty schema.validate(document)
+    end
+    should 'show url, last modified date, and content attributes' do
+      #visit items_sitemap_path


Layout/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)

houndci-bot · 2018-01-24T22:51:17Z

test/controllers/sitemap_controller_test.rb

+
+    should 'not show private items' do
+      #assert_select 'url', count: 2
+      assert_select 'loc', {count: 0, text: item_url(@private_item)}, 'private items shant appear in the sitemap'


Layout/SpaceInsideHashLiteralBraces: Space inside { missing. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
Layout/SpaceInsideHashLiteralBraces: Space inside } missing. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)

houndci-bot · 2018-01-24T22:51:17Z

test/controllers/sitemap_controller_test.rb

+    end
+
+    should 'not show private items' do
+      #assert_select 'url', count: 2


Layout/LeadingCommentSpace: Missing space after #. (https://github.com/bbatsov/ruby-style-guide#hash-space)

houndci-bot · 2018-01-24T22:51:18Z

test/controllers/sitemap_controller_test.rb

+      assert_select 'lastmod', @item.updated_at.to_s
+      @item.file_sets.first.unlock_and_fetch_ldp_object do |uo|
+        assert_select "rs|ln[href=?]", url_for(controller: :file_sets,
+                                                              action: :download,


Layout/AlignHash: Align the elements of a hash literal if they span more than one line.

houndci-bot · 2018-01-24T22:51:18Z

test/controllers/sitemap_controller_test.rb

+      assert_select 'loc', item_url(@item)
+      assert_select 'lastmod', @item.updated_at.to_s
+      @item.file_sets.first.unlock_and_fetch_ldp_object do |uo|
+        assert_select "rs|ln[href=?]", url_for(controller: :file_sets,


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. (https://github.com/bbatsov/ruby-style-guide#consistent-string-literals)

houndci-bot · 2018-01-24T22:51:18Z

test/controllers/sitemap_controller_test.rb

+        assert_select 'lastmod'
+        assert_select 'changefreq'
+        assert_select 'priority'
+        assert_select "rs|md[type=?]", 'text/html'


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. (https://github.com/bbatsov/ruby-style-guide#consistent-string-literals)

houndci-bot · 2018-01-24T22:51:19Z

test/controllers/sitemap_controller_test.rb

+                               publication_status: [CONTROLLED_VOCABULARIES[:publication_status].published],
+                               license: CONTROLLED_VOCABULARIES[:license].attribution_4_0_international,
+                               subject: ['Items'])
+        .unlock_and_fetch_ldp_object do |uo|


Layout/MultilineMethodCallIndentation: Align .unlock_and_fetch_ldp_object with .new_locked_ldp_object on line 39.

houndci-bot · 2018-01-24T22:51:19Z

test/controllers/sitemap_controller_test.rb

+                               item_type: CONTROLLED_VOCABULARIES[:item_type].article,
+                               publication_status: [CONTROLLED_VOCABULARIES[:publication_status].published],
+                               license: CONTROLLED_VOCABULARIES[:license].attribution_4_0_international,
+                               subject: ['Items'])


Layout/AlignHash: Align the elements of a hash literal if they span more than one line.

one failing bc Item.public is returning private items.

murny · 2018-01-29T20:32:07Z

config/environments/production.rb

@@ -86,4 +86,7 @@

  # Which ActiveStorage service to use
  config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] || :local).to_sym
+
+  # we can tell crawlers to go away during the beta launch phase
+  config.allow_crawlers = ENV['RAILS_ALLOW_CRAWLERS'].present?


For future reference:
Could have defined this in config/application.rb once instead of doing this in every environment especially when they are the same.

murny · 2018-01-29T20:34:24Z

test/controllers/resourcesync.xsd

@@ -0,0 +1,147 @@
+<?xml version="1.0" encoding="UTF-8"?>


Why does this resourcesync stuff living in test/controllers? Is it being used as a file? Maybe test/fixtures/file a better place?

murny · 2018-01-29T20:35:37Z

test/controllers/sitemap.xsd

@@ -0,0 +1,114 @@
+<?xml version="1.0" encoding="UTF-8"?>


Same for this one? Don't see anything reference these two files in your PR either (EDIT: I see sitemap.xsd being used but not resourcesync.xsd)?

murny · 2018-01-29T20:36:35Z

test/controllers/sitemap_controller_test.rb

+      get items_sitemap_url
+    end
+    should 'be valid sitemap xml' do
+      schema = Nokogiri::XML::Schema(File.open(File.join(File.dirname(__FILE__), 'sitemap.xsd')))


Ah your using it here. Yeah should look into file_fixtures and put them in test/fixtures/files

houndci-bot · 2018-01-29T20:45:16Z

config/environments/uat.rb

@@ -86,4 +86,5 @@

  # Which ActiveStorage service to use
  config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] || :local).to_sym
+


Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end. (https://github.com/bbatsov/ruby-style-guide#empty-lines-around-bodies)

houndci-bot · 2018-01-29T20:45:16Z

config/environments/test.rb

@@ -42,4 +42,5 @@

  # Which ActiveStorage service to use
  config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] || :test).to_sym
+


Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end. (https://github.com/bbatsov/ruby-style-guide#empty-lines-around-bodies)

houndci-bot · 2018-01-29T20:45:16Z

config/environments/staging.rb

@@ -83,4 +83,5 @@

  # Do not dump schema after migrations.
  config.active_record.dump_schema_after_migration = false
+


Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end. (https://github.com/bbatsov/ruby-style-guide#empty-lines-around-bodies)

houndci-bot · 2018-01-29T20:45:16Z

config/environments/production.rb

@@ -86,4 +86,5 @@

  # Which ActiveStorage service to use
  config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] || :local).to_sym
+


Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end. (https://github.com/bbatsov/ruby-style-guide#empty-lines-around-bodies)

houndci-bot · 2018-01-29T20:45:16Z

config/environments/development.rb

@@ -54,4 +54,5 @@

  # Which ActiveStorage service to use
  config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] || :local).to_sym
+


Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end. (https://github.com/bbatsov/ruby-style-guide#empty-lines-around-bodies)

murny · 2018-01-29T20:46:18Z

test/controllers/sitemap_controller_test.rb

+      get items_sitemap_url
+    end
+    should 'be valid sitemap xml' do
+      schema = Nokogiri::XML::Schema(File.open(Rails.root + 'test/fixtures/files/sitemap.xsd'))


Nokogiri isn't defined in our Gemfile (might be getting this from another gem, but regardless we should add it in case this other gem removes it down the road and now our test suite fails).

murny · 2018-01-29T20:48:21Z

test/controllers/sitemap_controller_test.rb

@@ -58,7 +58,7 @@ def before_all
      get sitemapindex_url
    end
    should 'be valid sitemapindex xml' do
-      schema = Nokogiri::XML::Schema(File.open(File.join(File.dirname(__FILE__), 'siteindex.xsd')))
+      schema = Nokogiri::XML::Schema(File.open(Rails.root + 'test/fixtures/files/siteindex.xsd'))


This can get even easier here. You should be able to just call:

file_fixture('siteindex.xsd')

http://api.rubyonrails.org/v5.1/classes/ActiveSupport/Testing/FileFixtures.html

murny · 2018-01-29T21:12:49Z

test/controllers/sitemap_controller_test.rb

+    end
+    should 'show location and last modified' do
+      assert_select 'loc', community_collection_url(@collection.community, @collection)
+      assert_select 'lastmod', @collection.updated_at.to_s\


Curious, why the \ at the end of updated_at.to_s? Happens a couple times in this file

Typo?! Copy-paste remnant.

murny · 2018-01-29T21:16:57Z

config/routes.rb

+  get 'sitemap-theses.xml', to: 'sitemap#theses', defaults: { format: :xml }
+
+  # Dynamic robots.txt
+  get 'robots.:format' => 'robots#robots'


Should we just limit it to robots.txt here instead of allowing any format extension? I assume it doesn't matter since in the controller action for this it only has a responds_to :text anyways?

Yeah, doesn't matter but the only thing that makes sense is robots.txt -- I'll change it.

murny · 2018-01-29T21:18:33Z

test/fixtures/files/resourcesync.xsd

@@ -0,0 +1,147 @@
+<?xml version="1.0" encoding="UTF-8"?>


Last comment I have on this PR, this file is never used anywhere? Is this something being kept around for future? Or If it's not needed just remove it?

Maybe I am wrong, looks like sitemap.xsd imports it maybe? From looks of it?

It is being used. It's included by the sitemap.xsd.

murny

Looks pretty good to me now.

mbarnett and others added 6 commits January 11, 2018 14:35

Add a basic cached sitemap. Some work remaining to flesh it out

5d14445

Make rubocop happy

408bd63

Cleanup

5f4d25a

sitemap doesn't need to have a policy, it's always public!

518dcb7

sitemap should only show public items

257aa21

add sitemap to robots.txt

d25bdd6

accesslint bot approved these changes Jan 15, 2018

View reviewed changes

pgwillia changed the title ~~Sitemap~~ Sitemap & Google Scholar Jan 15, 2018

Replaced placeholder with updated_at now indexed in Solr

7cdb9d2

pgwillia force-pushed the sitemap branch from e669958 to a52abcd Compare January 15, 2018 21:49

ResourceSync [http://www.openarchives.org/rs/1.1/]

8a9990f

pgwillia force-pushed the sitemap branch from a52abcd to 357f364 Compare January 15, 2018 21:50

murny reviewed Jan 15, 2018

View reviewed changes

pgwillia force-pushed the sitemap branch from 357f364 to c67d34c Compare January 16, 2018 21:00

https://scholar.google.ca/intl/en/scholar/inclusion.html#indexing

6523939

pgwillia force-pushed the sitemap branch from c67d34c to 6523939 Compare January 16, 2018 21:18

mbarnett mentioned this pull request Jan 16, 2018

[WIP] Add a basic cached sitemap. Some work remaining to flesh it out #340

Closed

6 tasks

pgwillia added 2 commits January 17, 2018 13:33

Merge branch 'master' into sitemap

bef291d

Update Google Scholar metadata & sitemap to accomodate Thesis

5a9dfa6

pgwillia force-pushed the sitemap branch 2 times, most recently from 7e30e59 to 88e13fc Compare January 17, 2018 23:12

sitemap should contain less than 50,000 targets

dd8aaca

pgwillia force-pushed the sitemap branch from 88e13fc to dd8aaca Compare January 17, 2018 23:21

mbarnett suggested changes Jan 18, 2018

View reviewed changes

mbarnett reviewed Jan 22, 2018

View reviewed changes

houndci-bot reviewed Jan 24, 2018

View reviewed changes

pgwillia added 2 commits January 25, 2018 13:38

WIP - notes from review and tests

0c54ebf

one failing bc Item.public is returning private items.

Merge branch 'master' into sitemap

9617268

pgwillia force-pushed the sitemap branch from 7497c12 to 9617268 Compare January 25, 2018 20:43

pgwillia added 4 commits January 25, 2018 14:37

Merge remote-tracking branch 'matt/composable_queries' into sitemap

cbc98e6

Make robots.txt dynamic based on ENV['RAILS_ALLOW_CRAWLERS']

a2d6bf8

move visibility constraints test to eliminate test order issues

508dfcc

finish sitemap testing

fac6395

pgwillia force-pushed the sitemap branch from fc70217 to fac6395 Compare January 26, 2018 19:58

pgwillia added 2 commits January 29, 2018 13:01

Merge branch 'master' into sitemap

09e4ec4

fix test from upstream change

c1b3344

murny reviewed Jan 29, 2018

View reviewed changes

houndci-bot reviewed Jan 29, 2018

View reviewed changes

murny reviewed Jan 29, 2018

View reviewed changes

pgwillia force-pushed the sitemap branch from 1084842 to 029c110 Compare January 29, 2018 20:47

murny reviewed Jan 29, 2018

View reviewed changes

pgwillia force-pushed the sitemap branch 3 times, most recently from 98f94f6 to d619391 Compare January 29, 2018 20:53

murny reviewed Jan 29, 2018

View reviewed changes

pgwillia force-pushed the sitemap branch from d619391 to d766f42 Compare January 29, 2018 21:16

murny reviewed Jan 29, 2018

View reviewed changes

learning about rails conventions

56e4d7e

pgwillia force-pushed the sitemap branch from d766f42 to 56e4d7e Compare January 29, 2018 21:26

murny approved these changes Jan 29, 2018

View reviewed changes

Merge branch 'master' into sitemap

bb89c15

mbarnett approved these changes Jan 30, 2018

View reviewed changes

pgwillia merged commit 5d3a5de into ualbertalib:master Jan 30, 2018

pgwillia deleted the sitemap branch March 9, 2018 03:56

pgwillia mentioned this pull request Nov 8, 2023

New reasons prevent pages in a sitemap from being indexed by Google on site https://era.library.ualberta.ca/ #3289

Open

		@@ -1,5 +1,7 @@
		<% page_title(@item.title) %>

		<%= render partial: 'google_scholar_metadata' %>

		@@ -1 +1,2 @@
		# See http://www.robotstxt.org/robotstxt.html for documentation on how to use the robots.txt file
		Sitemap: https://era.library.ualberta.ca/sitemap.xml

		@@ -86,4 +86,5 @@

		# Which ActiveStorage service to use
		config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] \|\| :local).to_sym

		@@ -42,4 +42,5 @@

		# Which ActiveStorage service to use
		config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] \|\| :test).to_sym

		@@ -83,4 +83,5 @@

		# Do not dump schema after migrations.
		config.active_record.dump_schema_after_migration = false

		@@ -54,4 +54,5 @@

		# Which ActiveStorage service to use
		config.active_storage.service = (ENV['ACTIVE_STORAGE_SERVICE'] \|\| :local).to_sym

Sitemap & Google Scholar #379

Sitemap & Google Scholar #379

Conversation

pgwillia commented Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbarnett Jan 18, 2018 • edited Loading

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

mbarnett commented Jan 18, 2018 • edited Loading

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

murny Jan 29, 2018 • edited Loading

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

pgwillia Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murny Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murny left a comment

Choose a reason for hiding this comment

pgwillia commented Jan 15, 2018 •

edited

Loading

mbarnett Jan 18, 2018 •

edited

Loading

mbarnett commented Jan 18, 2018 •

edited

Loading

murny Jan 29, 2018 •

edited

Loading

pgwillia Jan 29, 2018 •

edited

Loading

murny Jan 29, 2018 •

edited

Loading