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 a call to a milliner versioning endpoint for media #70

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

elizoller
Copy link
Member

@elizoller elizoller commented Feb 21, 2020

GitHub Issue: (Islandora/documentation#740)

What does this Pull Request do?

Passes the message from Drupal through to Milliner to create versions of Media in Fedora

What's new?

  • separates a Milliner version endpoint for Nodes and Media
  • fires an additional call to Milliner when its a new version of a Media

How should this be tested?

  • Because of the endpoint change in Milliner, it requires the related PR in Milliner (Media versioning Crayfish#96).
  • You'll need to clone this fork into your Islandora VM, build Alpaca, uninstall features, remove repo, add repo, install features -- see full details here: http://islandora.github.io/documentation/technical-documentation/alpaca_tips#steps-for-developing-with-alpaca
  • 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 a Milliner PR or it will break versioning for nodes (media versioning didn't work before this point). - Islandora/Crayfish#96

Interested parties

Tag @Islandora/8-x-committers

@codecov
Copy link

codecov bot commented Feb 21, 2020

Codecov Report

Merging #70 into dev will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##                dev   #70      +/-   ##
=========================================
+ Coverage     89.76%   90%   +0.23%     
  Complexity       73    73              
=========================================
  Files            10    10              
  Lines           293   300       +7     
  Branches          1     1              
=========================================
+ Hits            263   270       +7     
  Misses           29    29              
  Partials          1     1
Impacted Files Coverage Δ Complexity Δ
...slandora/alpaca/indexing/fcrepo/FcrepoIndexer.java 92.92% <100%> (+0.46%) 6 <0> (ø) ⬇️

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 a1dab02...5a3c9bf. Read the comment docs.

@dannylamb
Copy link
Contributor

I'm finally on this. Time to wrestle karaf...

@dannylamb
Copy link
Contributor

Okay, finally got this going and can verify it works with the Crayfish PR after a smoke test. I'll delve into the code now.

.toD(
getMillinerBaseUrl() +
"media/${exchangeProperty.sourceField}/version?connectionClose=true"
).endChoice();
Copy link
Contributor

@dannylamb dannylamb Sep 9, 2020

Choose a reason for hiding this comment

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

Funky indentation here

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean because i put 'getMillinerBaseUrl....' on the next line?
i'm pretty sure it was throwing an error about the line being too long.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was expecting the ending parenthesis before .endChoice to be aligned with .toD. Maybe that's just me.

No biggie though. It passes codestyle checks so I'm fine pulling in this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok i can fix it

"rel":"alternate"
}
],
"isNewVersion":"true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, shoulda caught this sooner, but "isNewVersion" should be down in the "attachment" section under "content" if we want to follow the schema to the letter of the law. Not the end of the world (thank goodness the AS2 police aren't a thing), but we should probably get in alignment with the ontology. I suppose this would affect node versioning as well. It's probably a lot to address here and now, so I'll make a ticket. There'll have to be work done in the islandora module to get this in the right place.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah ok, i was unaware of the schema for this

Copy link
Contributor

@dannylamb dannylamb left a comment

Choose a reason for hiding this comment

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

Aside from some odd indentation, I'm 👍 to this.

@dannylamb dannylamb merged commit 762a433 into Islandora:dev Sep 11, 2020
@elizoller
Copy link
Member Author

was i too late with my indentation fix?

@dannylamb
Copy link
Contributor

if you have it i'll take it and merge. i was just erring on the side of moving the code in. 🚀

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.

2 participants