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

Add OriginalFile display to IIF Manifest view #66

Merged
merged 3 commits into from
Jun 15, 2022
Merged

Add OriginalFile display to IIF Manifest view #66

merged 3 commits into from
Jun 15, 2022

Conversation

alxp
Copy link
Contributor

@alxp alxp commented May 2, 2022

What does this Pull Request do?

Add a display to the IIIF Manifest view that contains media tagged as OriginalFIle.

What's new?

Just a new display mode for IIF manifest.

How should this be tested?

  1. Check out this branch of islandora_defaults:

If islandora_defaults does not have a .git subfolder, do the following:

cd /var/www/html/drupal # or the equivalent if ISLE is different
composer remove islandora/islandora_defaults 
# You may need to agree to some prompts from composer at this point 
composer require --prefer-install=source islandora/islandora_defaults 
cd web/modules/contrib/islandora_defaults
git checkout iiif_original
  1. re-import islandora_defaults feature
$ drush fim -y islandora_defaults
  1. Enable islandora_mirador module
drush en islandora_mirador
  1. Create paged content:
    Repository item with model "Paged Content
    1 or more Repository items with model Page, and MemberOf relationship to the Paged Content item
    Media of type "File" for each of the Page items, containing a TIFF file.
  2. GO to the URL /node/[nid]/book-manifest where nid is the Paged Content item.Go to to verify existing behaviour, you should see some JSON describing the book structure.
  3. GO to the URL /node/[nid]/book-manifest-original where nid is the Paged Content item.
  4. Verify that the manifest contains references to the children's OriginalFile media.

Additional Notes:

Some types of files such as maps aren't served well by the JPG ServiceFile which is of limited resolution.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/8-x-committers

@ysuarez
Copy link
Contributor

ysuarez commented May 11, 2022

@alxp today I am starting the testing of this PR, and I have my first question. I will be using ISLE2 to test PR, and I am not 100% sure which ISLE2 "Make target" I should use to make sure I have met your testing requirement of importing islandora_defaults feature. In case you don't know I can ask in the "tech call" or Slack. Thanks.

@alxp
Copy link
Contributor Author

alxp commented May 11, 2022

Hi @ysuarez, thanks for taking a look at this.

You just need to be able to run a Drush command, so make demo may not be a good choice if I recall correctly. Make local or any other should b fine.

Let me know how you get on.

@ysuarez
Copy link
Contributor

ysuarez commented May 12, 2022

@alxp I just made my first attempt to test this, but I suspect I am doing something wrong or I am being tripped up by the isle-dc main branch still having some issues.

Here is what I tried so far....

  • make local
  • docker-compose exec drupal with-contenv bash
  • drush fim -y islandora_defaults

Then I got this error...

In FeaturesCommands.php line 758:
                                                    
  No such feature is available: islandora_defaults

Let me know what I need to do different or if you have any other suggestions. BTW, the only "content creation" options Drupal gave me were "Article" and "Basic Page," but I expected to have an option for "Resource Node" when using make local off of the development branch. Though I could be wrong.

@alxp
Copy link
Contributor Author

alxp commented May 12, 2022 via email

@ysuarez
Copy link
Contributor

ysuarez commented May 18, 2022

@alxp I just spent some time trying to check if islandora_defaults is enabled (after running make local), but I am not sure how since the islandora_defaults repo's installation section just links to a link about Drupal Features. (I can volunteer to at least a small update of those instructions once I sort testing this PR out.)

Since I am still learning about using and managing Drupal Features (with a capital "F"), I am not sure if isladnora_defaults can be installed the same way as a regular Drupal module. I did run find . -iname islandora_defaults on the Drupal container and got no hits.

BTW, my apologies for not knowing how to proceed. At this point I think I may need to run something like composer require islandora/islandora_defaults:2.x.x but I may still need to figure out how to pull down the version of your files from your PR. I can use wget and place it in whatever the appropriate path should be, something like web/modules/contrib/islandora

Let me know what you suggest. @seth-shaw-unlv do you have any suggestions for me?

@alxp
Copy link
Contributor Author

alxp commented May 18, 2022

Copying from Slack:

branch management in codebase is manual, but my guess is that you actually want to run composer require islandora/islandora_defaults:branch-dev and there's not a helper function for that, you just have to shell into the container and run it

That is the same as non-ISLE.

If you're still stuck let me know.

  • Alexander

@seth-shaw-unlv
Copy link
Contributor

So, @ysuarez, testing stuff in ISLE has been a bit of a pain. Most recently I've used Islandora-Devops/isle-dc#248 and then done make local-install-profile. This, at least, gave me a working site with a codebase folder to play with, although I needed to change the permissions for the module I was working on so git wouldn't complain.

I haven't gotten make local to do anything useful for me for quite a while.

If you want to use make demo you can get a bash session inside the Drupal container and just work in there, but that is less than ideal.

@ysuarez
Copy link
Contributor

ysuarez commented May 25, 2022

Quick update, today I ran make local and then I tried running...

composer require islandora/islandora_defaults:iif_original

but I guess I am putting in the wrong branch value, since I got this error...

  [UnexpectedValueException]                                                              
  Could not parse version constraint iif_original: Invalid version string "iif_original"  

@seth-shaw-unlv
Copy link
Contributor

Looks like you are missing an "i", and normally you would add "dev" to get a branch: composer require islandora/islandora_defaults:dev-iiif_original.

@alxp
Copy link
Contributor Author

alxp commented May 25, 2022

Hi @ysuarez

There's a slightly confusing thing with composer that you've run into:

The default place that composer looks for packages is on packages.org . There is an islandora registered namespace there, but it is not always up to date with non-head branches since it isn't directly part of GitHub.

The easiest way to get the branch you want is when you're logged in to the server, remove the existing islandora_defaults module and clone it from Github. Then you can check out the branch directly on the command line with Git.

The other option that plays more nicely with composer is to add a repositories: {} entry in composer but for short term testing it's not as necessary.

Best,

  • alexander

@ysuarez
Copy link
Contributor

ysuarez commented May 25, 2022

@alxp thanks for the explanation, it all makes sense. I can try the approach of removing existing then cloning islandora_defaults then switching to your PR's branch.

I am also curious about how to set up composer.json to pull the PR's branch, I will do some googling.

@ysuarez
Copy link
Contributor

ysuarez commented May 26, 2022

After running make local I tried cloning the islandora_defaults repo and had some issues, but more on that later.

I erased the cloned repo and went back to running...

composer require islandora/islandora_defaults:dev-iiif_original

This time making sure I had three the letter "i" in IIIF, since I know I originally caught that that I was missing an "i," but noticed that Seth mentioned it too. Looks like I did not properly combine the 3 "i" and "dev-" on Wednesday tests.

I still got an error, but maybe this one is an easier one to work around compared to manual repo cloning...

Info from https://repo.packagist.org: #StandWithUkraine
./composer.json has been updated
Running composer update islandora/islandora_defaults
Loading composer repositories with package information
Info from https://repo.packagist.org: #StandWithUkraine
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires islandora/islandora_defaults dev-iiif_original -> satisfiable by islandora/islandora_defaults[dev-iiif_original].
    - islandora/islandora_defaults dev-iiif_original requires islandora/islandora ^2 -> found islandora/islandora[2.0.0, ..., 2.x-dev] but it conflicts with your root composer.json require (dev-8.x-1.x).

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.

The current contents of the root composer.json's "require section for make local are...

    "require": {
        "composer/installers": "^1.9",
        "drupal/core-composer-scaffold": "^9.3",
        "drupal/core-project-message": "^9.3",
        "drupal/core-recommended": "^9.3",
        "drush/drush": "^10.3",
        "islandora/islandora": "dev-8.x-1.x"
    },

Thoughts?

@ysuarez
Copy link
Contributor

ysuarez commented May 27, 2022

I went back to running 'make local' and cloned the islandora_defaults repo in web/modules/contrib/islandora_defaults then switched to this PR's branch.

I was able to use composer to install the islandora_default dependencies

composer require drupal/geolocation
composer require drupal/field_group
composer require drupal/field_permissions
composer require islandora/openseadragon
composer require drupal/pdf
drush en islandora_defaults

I also tried running drush fim -y islandora_defaults too, but it might not have been necessary.

I will now attempt to finally test this PR, though let me know if anyone thinks I have done something wrong so far.

@ysuarez
Copy link
Contributor

ysuarez commented May 27, 2022

Here are the results of my testing tonight...

  • [parent node; nid=1] I created a "repository item" with model = "Paged Content"

  • [child A node; nid=2] I created another repository item" with model ="Digital Document"

    • with memberOf= [parent node]
    • with media of type = File; media use="original file"; uploaded TIFF file A
  • [child B node; nid=3] I created another repository item" with model ="Digital Document"

    • with memberOf= [parent node]
    • with media of type = File; media use="original file"; uploaded TIFF file B

At this URL....
https://islandora.traefik.me/node/1/book-manifest-original

The output is empty straight brackets...
[]

I may have done something wrong, so let me know what folks suggest.

BTW, one thing that I thought worked well in my "cloned repo" approach was that by using the current make local I was not given the install_profile, and therefore I did not have to set the "Resource Type." Which I would not have known which Resource Type values to use for this PR, if it would have mattered.

@alxp
Copy link
Contributor Author

alxp commented May 27, 2022

Hi @ysuarez ,

  1. Do you see any output at /node/x/book-manifest?
  2. Do you see any entries in the Drupal logs at /admin/reports/dblog?
  3. Do you see any entries in the apache error log when you load the page?

Thanks,

  • alexander

@ysuarez
Copy link
Contributor

ysuarez commented May 27, 2022

  • Do you see any output at /node/x/book-manifest?

No, also empty brackets for output.

  • Do you see any entries in the Drupal logs at /admin/reports/dblog?

I see some errors during the time I created the nodes and added the media files, but not sure if there is a smoking gun here or run-of-the-mill errors.

I am attaching a zipped copy of the HTML table that has the recent Drupal error messages, in case it is helpful. (Note: I needed to zip it to attach it to comment.)

PR 66 Drupal error log.html.zip

I am also adding a screenshot of what the recent Drupal errors look like.

image of Yamil's recent Drupal error logs for PR 66

@alxp or @seth-shaw-unlv, let me know if you want me to post the details of any of these log entries.

  • Do you see any entries in the apache error log when you load the page?

No, I am getting a status of 200 for both manifest URLs...

https://islandora.traefik.me/node/1/book-manifest-original
https://islandora.traefik.me/node/1/book-manifest

For the record, @alxp feel free to have someone more knowledgeable than me try to test this PR so it will take less time. Though I am happy 100% to keep trying new ways to test this PR properly, and have been learning a lot in the process.

@alxp
Copy link
Contributor Author

alxp commented Jun 3, 2022

HI @ysuarez , I've updated the testing instructions at the top of this ticket. A couple of things to note that may have caused it to not work for you:

  1. You need to enable islandora_mirador
  2. The child Repository Items should have the model 'Page', not 'Document'.

If you try again and have any trouble you can reach me on Slack and we can go through it together.

Thanks for your efforts on this.

  • alexander

@ysuarez
Copy link
Contributor

ysuarez commented Jun 8, 2022

@alxp quick update. I tried again using my previous ISLE2 instance from a couple of weeks ago. I just ran make up and ran drush en islandora_mirador without error. Though not sure if afterwards I need to run drush cr (which I eventually did) or some other command to make sure this module was installed properly. (Please advise.)

I then updated the model to "Page" for my existing child nodes and the two manifest URLs were still blank. Then I created a new parent node and two new child nodes (using memberOf) with the updated model instructions and still got empty manifests.

I can now try to build the instance again from scratch using make local, to again avoid the install_profile and the community theme in this case. Then manually install inslandora_defaults to the PR66 branch (and its module dependencies.)

Maybe it may be time to work together on Slack when I do this to see if you catch any issues I am creating or missing.

@ysuarez
Copy link
Contributor

ysuarez commented Jun 8, 2022

I just checked to see if one of the related Drupal views for this PR was installed/enabled, and it seems it has.

I visited...
https://islandora.traefik.me/admin/structure/views

and saw this...
image

@seth-shaw-unlv seth-shaw-unlv requested a review from ajstanley June 8, 2022 17:32
@ajstanley
Copy link
Contributor

@ysuarez - in your error log above it says it can't find book-manifesty - note the extra y

Copy link
Contributor

@ajstanley ajstanley left a comment

Choose a reason for hiding this comment

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

I built a shiny new box with local-install-profile, the only branch which has been reliably building for me lately.
I manually updated both Islandora and Islandora defaults, then switched to the iiif_original branch
The Mirador display context has to be set manually paged content nodes.
https://islandora.traefik.me/node/3/book-manifest-original shows the original file
https://islandora.traefik.me/node/3/book-manifest shows service files

@alxp alxp merged commit 641e5b8 into 2.x Jun 15, 2022
@alxp alxp deleted the iiif_original branch June 15, 2022 13:00
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.

5 participants