Mixed manifests provisional canvases and merging of manifests implementation#442
Merged
JackLewis-digirati merged 27 commits intodevelopfrom Aug 28, 2025
Merged
Conversation
d2a1593 to
3356bb8
Compare
7e4d043 to
35a3120
Compare
donaldgray
reviewed
Aug 12, 2025
Member
donaldgray
left a comment
There was a problem hiding this comment.
Couple of comments. I think items / item is overused in params and vars and more accurate naming might help clarify what's happening?
donaldgray
requested changes
Aug 15, 2025
Member
donaldgray
left a comment
There was a problem hiding this comment.
As discussed on call, couple of issues to resolve
- Invalid
canvas.Idon matched Canvas results in the Canvas being ignored (extra slug - https://localhost:7230/presentation/7/canvases/asimov) - Receiving
"Error creating DLCS space"when there was a general DLCS error. This was related to attempting to deletenullAssetId (maybe one for another ticket)? - General comment - too much reliance on
canvasOriginalId. We can accept different forms of CanvasId, which we reduce to the "id" part but we join oncanvasOriginalId, which could result in us joining 2 different forms. - Empty annotationPages (
"items": []) in a matched canvas breaks handling (check.IsNullOrEmpty()rather than== nullsomewhere?) GenerateProvisionalCanvas()- if matched canvas then we should add provisionalpaintingAnnotation. Linked to above, don't use canvasOriginalId for matching.
Don't rely on existance or not of annoPages in ManifestMerger Don't log dlcs request unless calling Protect against calling dlcs allImages if assetId null
…ngs based path rewrite parser + remove stopping manifests being switched between items and assets
…nd with finished canvases
…ctly in provisional canvases
cd8a17a to
3cba772
Compare
donaldgray
approved these changes
Aug 27, 2025
Member
donaldgray
left a comment
There was a problem hiding this comment.
LGTM - did some manual tests locally. Skimmed code due to size, couple of point raised but minor and mostly around comments.
JackLewis-digirati
added a commit
that referenced
this pull request
Oct 30, 2025
This pulls out the work done in #442 to ONLY include the section on rewritten path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #432
resolves #433
resolves #448
resolves #450
This PR modifies the provisional canvas paintings to work with the new mixed manifests feature.
Additionally, the manifest merger has been updated to provide final manifests that work with a mixture of items and painted resources
There has been a modification to settings to allow the settings based path generator to generate provisional paths, as opposed to the config generator. This has meant a reorganisation of settings and the api now implements the
presentationApiUrlsettings previously only available in the background handler.An example of this new settings block now looks like this:
Note
where these settings supersede settings with the same name in the previous iteration
Important
this PR will require changes to the terraform
Note
This PR also reverts changes from #290 which disallows manifests being created with either assets or items in the MVP