Ensures that Ephemera Projects have IIIF Manifests #385

jrgriffiniii · 2017-10-16T15:26:56Z

Resolves #364 by:

Ensuring that Ephemera Projects have IIIF Manifests (exposing Boxes)
Ensures that Ephemera Projects have slugs generated for them
Ensures that these slugs are published using RabbitMQ when EphemeraBoxes and EphemeraFolders are updated

tpendragon · 2017-10-16T20:08:40Z

app/change_sets/ephemera_project_change_set.rb

@@ -3,7 +3,7 @@ class EphemeraProjectChangeSet < Valkyrie::ChangeSet
  validates :title, presence: true
  property :title, multiple: false
  property :member_ids, multiple: true, required: false, type: Types::Strict::Array.member(Valkyrie::Types::ID)
-  property :slug, multiple: false, required: false
+  property :slug, multiple: false, required: true


This makes it required in the form, but there should be a validation too.

hackartisan

There are a lot of nice improvements to readability and idiomatic code in this PR.

I'm a bit confused about the slug. Why does it require a 4-digit suffix? How does the generated UUID relate to the object ID? Shouldn't there be a routing piece? What happens if a slug changes? Is there some 'preferred' or 'primary' slug, but routing will resolve any, i.e. they're all saved in the db and cannot be deleted? If these can simply be answered, perhaps do so in documentation on the model property.

hackartisan · 2017-10-17T14:45:38Z

app/assets/javascripts/ephemera_project_slug.js

+
+// UUID Generation without Bower, AMD's, or the NPM
+function uuidv4() {
+  return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {


does this become the ID of project, somehow?

Thank you for the review, documentation should indeed be added in order to clarify this feature.

Shouldn't there be a routing piece? What happens if a slug changes? Is there some 'preferred' or 'primary' slug [...]

This does not become the ID of the EphemeraProject. As the slug is entered by the user prior to ingestion (when the ID is assigned), this is simply a UUID-based alternative for generating unique slugs for EphemeraProject Objects.

A unique slug is required for the purposes of publishing messages via the RabbitMQ, but I don't believe that users ever actually reference or otherwise use this slug (from what I've gathered this is used for synchronization with other services such as https://github.com/pulibrary/lae-blacklight). Hence, I don't believe that routing or redirection should be offered. Please correct me if I am wrong @tpendragon or @escowles.

Unless there is a preference for users to generate the slug using their own scheme (i. e. if they happened to track these slugs for purposes of which I'm unaware), then I would think that the automatic generation using JavaScript is entirely unwarranted.

The slug is the user-readable shortname for the project/collection — e.g., for use in DPUL, where the collection URL is dpul.princeton.edu/[slug]. So it should be entered by the user, and shouldn't be auto-generated or have any suffixes, etc. to make sure it's unique. If there is another collection or ephemera project with the slug, an we should just display an error message to the user.

Ah, okay, my understanding was that this would be the basis of a distributable URL to aid in replacing standalone sites like the one for lae with easy-to-access collection pages within figgy/plum. Was I mistaken here? @escowles? Totally possible, considering I didn't know about the messaging use case.

@HackMasterA I think that's right, except the collection pages are in DPUL, not figgy/plum. The point of the slug property is for plum/figgy to tell DPUL what it should use for the collection shortname for building its URLs.

Okay, but we don't need routing because it's not really for use in figgy / plum -- it's just for use in DPUL.

jrgriffiniii · 2017-10-17T16:42:55Z

This has been updated and is prepared for another review.

tpendragon · 2017-10-17T17:05:02Z

app/change_sets/ephemera_project_change_set.rb

  end
+
+  def validate(resource_params = nil)


I don't think we should override validate. Can you just add a custom method validation instead via something like validate :slug_is_unique?

hackartisan · 2017-10-17T18:12:52Z

While you're adding a validation for uniqueness I would also like to see a validation for suitability in a url. like validate :slug_characters or something that ensures it's only lowercase alphanumeric plus dashes.

hackartisan · 2017-10-17T18:21:26Z

app/decorators/ephemera_project_decorator.rb

@@ -25,4 +29,20 @@ def manageable_structure?
  def title
    super.first
  end
+
+  def slug
+    Array.wrap(super).first


If we want it to be single-value, we should enforce that on the changeset. But I'm not sure we want it to be single-value. I'm concerned we actually need 2 properties, like current_slug and all_slugs, so that the front-end app can continue routing to old / bookmarked urls. I think this may need more discussion / a new issue.

Oh, it already is enforced on the changeset, okay. Well the rest of my concern still stands :)

I've created a new issue for handling changed slugs gracefully — not sure if it's ultimately something that needs to change in DPUL or Figgy, but it's something that should work: #395

jrgriffiniii · 2017-10-17T20:58:30Z

Slug validation has been improved upon as requested and this is prepared for another review.

hackartisan · 2017-10-17T20:59:25Z

Looks good to me!

…exposing Boxes as Manifests; Ensures that Ephemera Projects have slugs generated for them; Ensures that these slugs are published using RabbitMQ when EphemeraBoxes and EphemeraFolders are updated

…or cases where the slug is generated, use the "title" property in order to generate the slug prefix Ensuring that the "slug" property for the EphemeraProjectChangeSet is present; Adding a JavaScript snippet for populating the "Slug" field within the EphemeraProject form

…s are unique before they are persisted

…sing a custom validation method rather than simply overridding #validate

… characters

jrgriffiniii added the in progress label Oct 16, 2017

jrgriffiniii force-pushed the issues-364-jrgriffiniii-ephemera-project-manifest branch from 9f5ca17 to 73b67a3 Compare October 16, 2017 19:05

tpendragon requested changes Oct 16, 2017

View reviewed changes

jrgriffiniii force-pushed the issues-364-jrgriffiniii-ephemera-project-manifest branch 3 times, most recently from 9e08a41 to 4b9ab1c Compare October 17, 2017 14:29

hackartisan reviewed Oct 17, 2017

View reviewed changes

jrgriffiniii changed the title ~~Ensures that Ephemera Projects have IIIF Manifests~~ [WIP] Ensures that Ephemera Projects have IIIF Manifests Oct 17, 2017

jrgriffiniii changed the title ~~[WIP] Ensures that Ephemera Projects have IIIF Manifests~~ Ensures that Ephemera Projects have IIIF Manifests Oct 17, 2017

tpendragon requested changes Oct 17, 2017

View reviewed changes

hackartisan reviewed Oct 17, 2017

View reviewed changes

jrgriffiniii force-pushed the issues-364-jrgriffiniii-ephemera-project-manifest branch from c5af25b to fbb8bc6 Compare October 17, 2017 19:31

jrgriffiniii changed the title ~~Ensures that Ephemera Projects have IIIF Manifests~~ [WIP] Ensures that Ephemera Projects have IIIF Manifests Oct 17, 2017

jrgriffiniii force-pushed the issues-364-jrgriffiniii-ephemera-project-manifest branch from fbb8bc6 to fa28089 Compare October 17, 2017 20:39

jrgriffiniii changed the title ~~[WIP] Ensures that Ephemera Projects have IIIF Manifests~~ Ensures that Ephemera Projects have IIIF Manifests Oct 17, 2017

jrgriffiniii added 6 commits October 18, 2017 09:55

Resolves #346 by ensuring that Ephemera Projects have IIIF Manifests …

c7ae5e2

…exposing Boxes as Manifests; Ensures that Ephemera Projects have slugs generated for them; Ensures that these slugs are published using RabbitMQ when EphemeraBoxes and EphemeraFolders are updated

Removing Slug generation functionality from the EphemeraProjectDecorator

a6fed8f

Removing the JavaScript auto-generation for slugs; Ensuring that slug…

6384f6d

…s are unique before they are persisted

Checking for unique slug values within the EphemeraProjectChangeSet u…

5336c5e

…sing a custom validation method rather than simply overridding #validate

Extending the EphemeraProjectChangeSet to include validation for slug…

418a25d

… characters

jrgriffiniii force-pushed the issues-364-jrgriffiniii-ephemera-project-manifest branch from fa28089 to 418a25d Compare October 18, 2017 13:55

Upgrading to Ruby release 2.3.4 for the CircleCI build environment

db322e9

jrgriffiniii added Review and removed in progress labels Oct 18, 2017

tpendragon approved these changes Oct 18, 2017

View reviewed changes

tpendragon merged commit 4eeb873 into master Oct 18, 2017

tpendragon removed the Review label Oct 18, 2017

tpendragon deleted the issues-364-jrgriffiniii-ephemera-project-manifest branch October 18, 2017 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensures that Ephemera Projects have IIIF Manifests #385

Ensures that Ephemera Projects have IIIF Manifests #385

jrgriffiniii commented Oct 16, 2017 •

edited

tpendragon Oct 16, 2017

hackartisan left a comment

hackartisan Oct 17, 2017

jrgriffiniii Oct 17, 2017 •

edited

escowles Oct 17, 2017

hackartisan Oct 17, 2017

escowles Oct 17, 2017

hackartisan Oct 17, 2017

jrgriffiniii commented Oct 17, 2017

tpendragon Oct 17, 2017

hackartisan commented Oct 17, 2017

hackartisan Oct 17, 2017

hackartisan Oct 17, 2017

escowles Oct 17, 2017

jrgriffiniii commented Oct 17, 2017

hackartisan commented Oct 17, 2017

Ensures that Ephemera Projects have IIIF Manifests #385

Ensures that Ephemera Projects have IIIF Manifests #385

Conversation

jrgriffiniii commented Oct 16, 2017 • edited

Choose a reason for hiding this comment

hackartisan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgriffiniii Oct 17, 2017 • edited

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

jrgriffiniii commented Oct 17, 2017

Choose a reason for hiding this comment

hackartisan commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgriffiniii commented Oct 17, 2017

hackartisan commented Oct 17, 2017

jrgriffiniii commented Oct 16, 2017 •

edited

jrgriffiniii Oct 17, 2017 •

edited