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

Moving OBJ above derivatives. #153

Merged
merged 1 commit into from Jan 8, 2016
Merged

Moving OBJ above derivatives. #153

merged 1 commit into from Jan 8, 2016

Conversation

whikloj
Copy link
Member

@whikloj whikloj commented Nov 24, 2015

Partially addresses ISLANDORA-1527

What does this Pull Request do?

Re-order datastreams to allow proper derivative creation when batch ingesting large images

How should this be tested?

Batch ingest one or more large images using drush (islandora_batch_scan_preprocess).
Before the derivatives (TN, JP2) files were not generated after ingest.
Now they should be generated once the objects are ingested.

@whikloj
Copy link
Member Author

whikloj commented Nov 24, 2015

@nhart If you want to have a look to make sure I'm doing this correctly. 😄

@ruebot
Copy link
Member

ruebot commented Dec 10, 2015

@whikloj looks like we need to resolve merge conflicts.

Once that is done, @nhart, are you still willing to review and merge this?

@ruebot
Copy link
Member

ruebot commented Jan 7, 2016

@nhart bump

@whikloj looks like we have merge conflicts 😢

@whikloj
Copy link
Member Author

whikloj commented Jan 8, 2016

Rebased and ready to go.

ruebot added a commit that referenced this pull request Jan 8, 2016
@ruebot ruebot merged commit f64659e into Islandora:7.x Jan 8, 2016
@nhart
Copy link
Contributor

nhart commented Jan 8, 2016

I didn't test this, did you @ruebot? Don't see anything mentioned in the pr comments or ticket about testing results.

@adam-vessey
Copy link
Contributor

(Just updating an environment, and checking out the changes...)

Also, looks like this might be effecting/undoing one of @rosiel's changes, since it no longer has the optional flag.

@ruebot
Copy link
Member

ruebot commented Jan 11, 2016

Shall we revert? Or should we put in an updated pull request?

@ruebot
Copy link
Member

ruebot commented Jan 11, 2016

...that said, I'm happy to put in a quick pull request since I merged this one.

@adam-vessey
Copy link
Contributor

Somewhat ambivalent, am I... I'm not sure it should be there (or if present, should be optional="false")... Should the DS-COMPOSITE-MODEL document the final, fully-populated state of the object, or any (or all?) states of the object?

@ruebot
Copy link
Member

ruebot commented Jan 11, 2016

So we're consistent with the other solution packs, we should have optional="true" in there, and I'll own it and take the blame for not catching that when I merged. I should have known better since I tested and merged most of @rosiel's work around making OBJ datastreams optional.

So, I'll do a quick pull request to add optional="true" to this to fix it, and just associated it with this pull request and existing JIRA ticket.

@rosiel
Copy link
Member

rosiel commented Jan 11, 2016

Thanks for catching this!

@adam-vessey I agree with you. I'm not sure about the philosophical meaning of the DS_COMPOSITE_MODEL but it was suggested to change this when making the upload optional in the ingest forms. Previously we've only made optional any derivative datastream that system could create (or not) while the payload was always OBJ and it was assumed that this was the preservation target.

Are there use cases where a final, fully populated object would not have an OBJ (when it is of one of these standard solution pack content models)? Perhaps we should discuss this on the message board. Until this is resolved, thanks for maintaining consistency.

@whikloj
Copy link
Member Author

whikloj commented Jan 11, 2016

Thanks all, that was my mistake after the rebase of this PR on recent changes.

rosiel added a commit that referenced this pull request Jan 11, 2016
Update DS-COMPOSITE per post merge code review on #153.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants