Media versioning #96

elizoller · 2020-02-21T04:35:53Z

GitHub Issue: (Islandora/documentation#740)

Other Relevant Links
add a call to a milliner versioning endpoint for media Alpaca#70

What does this Pull Request do?

Allows Milliner to fire requests to Fedora to create versions of Media objects

What's new?

Separates the routes for node versioning and media versioning - because they rely on different data
Abstracts shared code with the saveMedia method where possible to reduce duplication
Creates a Fedora version for a Media object

How should this be tested?

Bring in the related Alpaca PR here: add a call to a milliner versioning endpoint for media Alpaca#70
Clone/pull this PR into Milliner (in playbook - /var/www/html/Crayfish/Milliner)
Create a repository item node (make sure it persists to Fedora)
Edit the node - make sure it gets a version in Fedora - in Fedora UI or via API call
Create a media object (make sure it persists to Fedora)
Edit that media object - make sure it gets a version in Fedora (not available through Fedora UI, have to call like http://127.0.0.1:8080/fcrepo/rest/2020-02/file.pdf/fcr:metadata/fcr:versions

Additional Notes:

This PR depends on an Alpaca PR or it will break versioning for nodes (media versioning didn't work before this point). Islandora/Alpaca#70

Interested parties

@Islandora/8-x-committers

… a helper method for getting the file from the media, flesh out the method for creating a media version

…method in the service instead

…andora#92) Bumps [symfony/http-foundation](https://github.com/symfony/http-foundation) from 3.4.30 to 3.4.36. - [Release notes](https://github.com/symfony/http-foundation/releases) - [Changelog](https://github.com/symfony/http-foundation/blob/master/CHANGELOG.md) - [Commits](symfony/http-foundation@v3.4.30...v3.4.36) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ndora#94) Bumps [symfony/http-foundation](https://github.com/symfony/http-foundation) from 3.4.30 to 3.4.36. - [Release notes](https://github.com/symfony/http-foundation/releases) - [Changelog](https://github.com/symfony/http-foundation/blob/master/CHANGELOG.md) - [Commits](symfony/http-foundation@v3.4.30...v3.4.36) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ora#91) Bumps [symfony/http-foundation](https://github.com/symfony/http-foundation) from 3.4.30 to 3.4.36. - [Release notes](https://github.com/symfony/http-foundation/releases) - [Changelog](https://github.com/symfony/http-foundation/blob/master/CHANGELOG.md) - [Commits](symfony/http-foundation@v3.4.30...v3.4.36) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ora#101) Bumps [symfony/http-foundation](https://github.com/symfony/http-foundation) from 3.4.30 to 3.4.38. - [Release notes](https://github.com/symfony/http-foundation/releases) - [Changelog](https://github.com/symfony/http-foundation/blob/master/CHANGELOG.md) - [Commits](symfony/http-foundation@v3.4.30...v3.4.38) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…dora#98) Bumps [symfony/http-foundation](https://github.com/symfony/http-foundation) from 3.4.30 to 3.4.38. - [Release notes](https://github.com/symfony/http-foundation/releases) - [Changelog](https://github.com/symfony/http-foundation/blob/master/CHANGELOG.md) - [Commits](symfony/http-foundation@v3.4.30...v3.4.38) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

dannylamb

Other than some small things, this looks like a good little refactor. We'll have to sort out what's up with tests, too, but I'm 👍 to this.

dannylamb · 2020-09-08T20:09:13Z

Milliner/src/Controller/MillinerController.php

    {
        $token = $request->headers->get("Authorization", null);
        try {
+            $this->log->info("in create Node version with " . $uuid);


I would delete this or set the level to debug.

dannylamb · 2020-09-08T20:09:33Z

Milliner/src/Controller/MillinerController.php

    {
        $token = $request->headers->get("Authorization", null);
        try {
+            $this->log->info("in create Node version with " . $uuid);
+            $urls = $this->milliner->getGeminiUrls($uuid, $token);
+            $this->log->info($urls);


I would delete these two lines or set the levels to debug.

dannylamb · 2020-09-08T20:10:47Z

Milliner/src/Controller/MillinerController.php

+        $json_url = $request->headers->get("Content-Location");
+
+        if (empty($json_url)) {
+            $this->log->info("json url is EMPTY");


I would use error instead of info here

dannylamb · 2020-09-08T20:11:09Z

Milliner/src/Controller/MillinerController.php

+            $this->log->info("json url is EMPTY");
+            return new Response("Expected JSON url in Content-Location header", 400);
+        } else {
+            $this->log->info("json url is" . $json_url);


I'd make this debug or delete it

dannylamb · 2020-09-08T20:12:03Z

Milliner/src/Service/MillinerService.php

-                    $timestamp,
-                    null,
-                    $headers
+        $this->log->error("the fedora url in service is " . $fedora_url);


I'd make this debug or delete it.

dannylamb · 2020-09-08T20:25:28Z

Milliner/src/Service/MillinerService.php

+                404
+            );
+        }
+        return array('fedora'=>$urls['fedora'], 'jsonld' =>$jsonld_url, 'drupal'=>$urls['drupal']);


I hate myself for nitpicking on this, but you should use [] instead of array here

dannylamb · 2020-09-08T20:34:58Z

Hrm... Chullo is out of date at 30f13e7 according to the composer install output from Travis, but it is recent enough to contain versioning. It's not a bad idea to run composer update and commit the updated lockfile just in case.

… a helper method for getting the file from the media, flesh out the method for creating a media version

…method in the service instead

…ayfish into media_versioning

elizoller · 2020-09-09T17:46:48Z

I might need some help with the tests here: https://travis-ci.com/github/Islandora/Crayfish/jobs/382853022

dannylamb · 2020-09-10T18:30:25Z

Because you refactored and added the getGeminiUrls and getFileFromMedia functions, you need to wire up your mocks to return values for them or throw exceptions. Here's a diff of what I had to do to get them working.

diff --git a/Milliner/tests/Islandora/Milliner/Tests/MillinerControllerTest.php b/Milliner/tests/Islandora/Milliner/Tests/MillinerControllerTest.php
index 09b9e74..e28e5f9 100644
--- a/Milliner/tests/Islandora/Milliner/Tests/MillinerControllerTest.php
+++ b/Milliner/tests/Islandora/Milliner/Tests/MillinerControllerTest.php
@@ -48,12 +48,16 @@ class MillinerControllerTest extends TestCase
         $milliner = $this->prophesize(MillinerServiceInterface::class);
         $milliner->saveNode(Argument::any(), Argument::any(), Argument::any(), Argument::any())
             ->willThrow(new \Exception("Forbidden", 403));
+        $milliner->getFileFromMedia(Argument::any(), Argument::any(), Argument::any())
+            ->willThrow(new \Exception("Forbidden", 403));
         $milliner->saveMedia(Argument::any(), Argument::any(), Argument::any())
             ->willThrow(new \Exception("Forbidden", 403));
         $milliner->deleteNode(Argument::any(), Argument::any())
             ->willThrow(new \Exception("Forbidden", 403));
         $milliner->saveExternal(Argument::any(), Argument::any(), Argument::any(), Argument::any())
             ->willThrow(new \Exception("Forbidden", 403));
+        $milliner->getGeminiUrls(Argument::any(), Argument::any())
+            ->willThrow(new \Exception("Forbidden", 403));
         $milliner->createVersion(Argument::any(), Argument::any())
             ->willThrow(new \Exception("Forbidden", 403));
         $milliner = $milliner->reveal();
@@ -473,7 +477,9 @@ class MillinerControllerTest extends TestCase
     public function testCreateNodeVersionReturnsSuccessOnSuccess()
     {
         $milliner = $this->prophesize(MillinerServiceInterface::class);
-        $milliner->createVersion(Argument::any(), Argument::any(), Argument::any(), Argument::any())
+        $milliner->getGeminiUrls(Argument::any(), Argument::any())
+            ->willReturn(['fedora' => "http://example.org/fcrepo/abc123", "drupal" => "http://example.org/node/1"]);
+        $milliner->createVersion(Argument::any(), Argument::any())
             ->willReturn(new Response(201));
         $milliner = $milliner->reveal();
         $controller = new MillinerController($milliner, $this->logger);
@@ -499,7 +505,9 @@ class MillinerControllerTest extends TestCase
         );
 
         $milliner = $this->prophesize(MillinerServiceInterface::class);
-        $milliner->createVersion(Argument::any(), Argument::any(), Argument::any(), Argument::any())
+        $milliner->getGeminiUrls(Argument::any(), Argument::any())
+            ->willReturn(['fedora' => "http://example.org/fcrepo/abc123", "drupal" => "http://example.org/node/1"]);
+        $milliner->createVersion(Argument::any(), Argument::any())
             ->willReturn(new Response(204));
         $milliner = $milliner->reveal();
         $controller = new MillinerController($milliner, $this->logger);

elizoller · 2020-09-10T19:29:55Z

my hero!!!

codecov · 2020-09-10T19:32:57Z

Codecov Report

Merging #96 into dev will decrease coverage by 2.98%.
The diff coverage is 58.82%.

@@             Coverage Diff              @@
##                dev      #96      +/-   ##
============================================
- Coverage     94.85%   91.87%   -2.99%     
- Complexity      168      175       +7     
============================================
  Files             9        9              
  Lines           700      726      +26     
============================================
+ Hits            664      667       +3     
- Misses           36       59      +23

Impacted Files	Coverage Δ	Complexity Δ
Milliner/src/Service/MillinerService.php	`89.84% <43.18%> (-9.37%)`	`56.00 <6.00> (+2.00)`	⬇️
Milliner/src/Controller/MillinerController.php	`97.02% <87.50%> (+2.09%)`	`24.00 <8.00> (+5.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 463aa7e...291ebe0. Read the comment docs.

elizoller added 3 commits February 20, 2020 21:17

add separate routes in milliner for versioning nodes and media

d46ce38

separate createVersion for node and Media as much as possible, create…

620b5e4

… a helper method for getting the file from the media, flesh out the method for creating a media version

remove excess log statements

e7338ce

elizoller mentioned this pull request Feb 21, 2020

add a call to a milliner versioning endpoint for media Islandora/Alpaca#70

Merged

elizoller added 3 commits February 20, 2020 21:38

code cleanup

6a65e3c

fix node versioning - no gemini from the controller so made a helper …

efd2203

…method in the service instead

adding and updating tests - still some of these fail

419d1f3

elizoller mentioned this pull request Mar 12, 2020

Versioning / History asulibraries/islandora-repo#5

Closed

dependabot bot and others added 6 commits March 26, 2020 14:56

Update CONTRIBUTING.md (Islandora#104)

4252083

seth-shaw-unlv requested a review from dannylamb August 26, 2020 17:58

dannylamb added 2 commits August 27, 2020 17:26

Update .travis.yml (Islandora#105)

37b95bb

Master -> Main Rename (Islandora#106)

e93f569

dannylamb requested changes Sep 8, 2020

View reviewed changes

elizoller and others added 11 commits September 9, 2020 05:44

removing unnecessary logging

d694430

update Milliner composer dependencies

31367b2

add separate routes in milliner for versioning nodes and media

6abec98

separate createVersion for node and Media as much as possible, create…

fd24812

… a helper method for getting the file from the media, flesh out the method for creating a media version

remove excess log statements

d11815f

code cleanup

7e23fbd

fix node versioning - no gemini from the controller so made a helper …

5737cc2

…method in the service instead

adding and updating tests - still some of these fail

352043b

removing unnecessary logging

9b4c607

update Milliner composer dependencies

257728c

Merge branch 'media_versioning' of https://github.com/asulibraries/cr…

54b1170

…ayfish into media_versioning

elizoller added 2 commits September 9, 2020 13:14

fix erroring test

40b4e34

fix spacing in test

853ecec

update tests - copy paste from @dannylamb

291ebe0

dannylamb approved these changes Sep 11, 2020

View reviewed changes

dannylamb merged commit de6c968 into Islandora:dev Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media versioning #96

Media versioning #96

elizoller commented Feb 21, 2020 •

edited

Loading

dannylamb left a comment

dannylamb Sep 8, 2020

dannylamb Sep 8, 2020

dannylamb Sep 8, 2020

dannylamb Sep 8, 2020

dannylamb Sep 8, 2020

dannylamb Sep 8, 2020

dannylamb commented Sep 8, 2020

elizoller commented Sep 9, 2020

dannylamb commented Sep 10, 2020

elizoller commented Sep 10, 2020

codecov bot commented Sep 10, 2020

Media versioning #96

Media versioning #96

Conversation

elizoller commented Feb 21, 2020 • edited Loading

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

dannylamb left a comment

Choose a reason for hiding this comment

dannylamb Sep 8, 2020

Choose a reason for hiding this comment

dannylamb Sep 8, 2020

Choose a reason for hiding this comment

dannylamb Sep 8, 2020

Choose a reason for hiding this comment

dannylamb Sep 8, 2020

Choose a reason for hiding this comment

dannylamb Sep 8, 2020

Choose a reason for hiding this comment

dannylamb Sep 8, 2020

Choose a reason for hiding this comment

dannylamb commented Sep 8, 2020

elizoller commented Sep 9, 2020

dannylamb commented Sep 10, 2020

elizoller commented Sep 10, 2020

codecov bot commented Sep 10, 2020

Codecov Report

elizoller commented Feb 21, 2020 •

edited

Loading