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

Create digital resources for the manifests and the canvases, #1438 #1439

Merged
merged 20 commits into from
Nov 21, 2023

Conversation

njkim
Copy link
Contributor

@njkim njkim commented Nov 11, 2023

Add receiver to create digital resources resource instances for the manifests and the canvases.
The pr is dependent on archesproject/arches#10261

Three db tables are added:

  • manifest_x_digitalresource
  • canvas_x_digitalresource
  • manifest_x_canvas

If manifest is added:

  • digital resource for manifest and manifest_x_digitalresource record are created
  • digital resources for canvases and canvas_x_digitalresource records are created
  • manifest_x_canvas records are created

If manifest is deleted:

  • manifest_x_digitalresource record is deleted (note, the digital resource is NOT deleted)
  • manifest_x_canvas records are deleted

If canvas is added to a manifest

  • digital resources for canvas and canvas_x_digitalresource record are created (if the canvas is new)
  • manifest_x_canvas record is created

If canvas is removed from a manifest

  • manifest_x_canvas is deleted (again note the digital resources are NOT deleted)

Note,

  • The digital resource: once it is created it won't be deleted automatically
  • canvas_x_digitalresource: it won't be deleted, either.
  • manifest_x_digitalresource, manifest_x_canvas: they are updated as the manifest and the canvas configuration are updated

@njkim njkim requested a review from chiatt November 14, 2023 08:59
Copy link
Member

@chiatt chiatt left a comment

Choose a reason for hiding this comment

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

Just a few minor fixes to make

self.saveSamplingActivityDigitalReference(data.resourceinstance_id);
$.getJSON( arches.urls.api_card + digitalResourcesResourceId )
.then(function(data) {
console.log(data);
Copy link
Member

Choose a reason for hiding this comment

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

Remove log statement

@@ -240,7 +249,7 @@ define([
});
};

this.getThumnail = function(digitalResourceData) {
this.getThumbnail = function(digitalResourceData) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

try:
statement_tile = Tile.objects.filter(nodegroup_id=statement_nodegroupid, resourceinstance_id=resource_id)[0]
except:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this except ObjectDoesNotExist?

manifest_data = instance.manifest
if created:
create_manifest_digitla_resource(manifest_data, instance.transactionid)
Copy link
Member

Choose a reason for hiding this comment

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

There's a type in 'digital'

arches_for_science/models.py Show resolved Hide resolved
@receiver(post_save, sender=IIIFManifest)
def create_digital_resources(sender, instance, created, **kwargs):
from arches_for_science.utils.digital_resource_for_manifest import digital_resources_for_manifest, digital_resources_for_canvases
Copy link
Member

Choose a reason for hiding this comment

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

Do these imports not work from the top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The functions are importing the models (from this file). So, I didn't find a way to avoid the circular import.

arches_for_science/utils/digital_resource_for_manifest.py Outdated Show resolved Hide resolved
Copy link
Member

@chiatt chiatt left a comment

Choose a reason for hiding this comment

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

This is looking really good @njkim. It works great for me in the manifest manager. In the Sample Taking workflow the digital resources are created as expected, but I get the following error when trying to save and continue through the workflow:
image

Comment on lines +55 to +56
digital_resources_for_canvases(instance)
Copy link
Member

Choose a reason for hiding this comment

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

This should be wrapped in a transaction

@receiver(post_delete, sender=IIIFManifest)
def delete_manifest_x_canvas(sender, instance, **kwargs):
ManifestXCanvas.objects.filter(manifest=instance.manifest["@id"]).delete()
Copy link
Member

Choose a reason for hiding this comment

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

Probably also good to wrap in a transaction

Comment on lines 117 to 119
"""
Creates the digital resources resource instance representing manifest
and also creates the manifest_x_canvas record
Copy link
Member

Choose a reason for hiding this comment

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

Nice that you added the doc strings, but we should format them like this: https://peps.python.org/pep-0257/#multi-line-docstrings

Comment on lines 176 to 178
"""
the main function to crate/update the digital resource for the manifest
"""
Copy link
Member

Choose a reason for hiding this comment

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

One line doc strings like this: https://peps.python.org/pep-0257/#one-line-docstrings
Also, create has a small typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, thanks.


def digital_resources_for_canvases(instance):
"""
the main function to crate/update the digital resource for the canvases
Copy link
Member

Choose a reason for hiding this comment

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

Small typo in 'create'

@njkim njkim requested a review from chiatt November 20, 2023 22:42
@chiatt chiatt merged commit 1152dab into dev/1.1.x Nov 21, 2023
1 check passed
@chiatt chiatt deleted the 1438_digital_resources_manifest_canvas branch November 21, 2023 00:53
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