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

Delete check parent links #124

Merged
merged 4 commits into from
Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,29 @@ OME.truncateNames = (function(){
return truncateNames;
}());

OME.checkForMultipleParents = function(objList, callback) {
// objList is list of ['image=1'] etc.
var url = WEBCLIENT.URLS.api_parent_links + '?' + objList.join('&');

$.getJSON(url, function(data){
// look for any child that has > 1 parent:
var multiple = false;
var links = {};
data.data.forEach(function(link){
var childId = link.child.type + link.child.id;
Copy link
Member

Choose a reason for hiding this comment

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

if I understand the dialog correctly as soon as one image has more than one parent the warning is displayed. So the loop could be stop as soon as multiple is true

Copy link
Member Author

Choose a reason for hiding this comment

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

If I want to break out of the loop early then I have to switch to using a for loop, which is slightly less nice coding style (I prefer the 'functional' style). I don't think you will notice much performance improvement to break early but can refactor if you want?

Copy link
Member

Choose a reason for hiding this comment

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

i understand but we have seen cases where people were trying to delete a large number of objects from the UI
so it will be good to see if it makes a major change in performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is running after we make a call to the server, which takes e.g. 162 millisecs for 46 images (just tried that). I imagine the refactor will save maybe a few millisecs and won't be noticed compared to the time to load the data. With a large number of images the effect will be even smaller since it will take longer to load the data.
If you want me to spend time to analyse the performance under various conditions (for loop vv forEach and many images where the first has multiple parents vv many images where none of the images have multiple parents (so we don't break early) then I can do, but I think that would not be the best use of time.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking

If you want me to spend time to analyse the performance under various conditions (for loop vv forEach and many images where the first has multiple parents vv many images where none of the images have multiple parents (so we don't break early) then I can do, but I think that would not be the best use of time.

This is not what I was suggesting. Nonetheless we need to keep in mind performance and large volume since it is the reality! and break early if required.
For this call, it seems that we might not have a significant performance degradation.

var parentId = link.parent.type + link.parent.id;
// do we already have a parent link for that child?
if (links[childId]) {
multiple = true;
}
links[childId] = parentId;
});
if (callback) {
callback(multiple);
}
});
}

// Handle deletion of selected objects in jsTree in containers.html
OME.handleDelete = function(deleteUrl, filesetCheckUrl, userId) {
var datatree = $.jstree.reference($('#dataTree'));
Expand All @@ -543,19 +566,6 @@ OME.handleDelete = function(deleteUrl, filesetCheckUrl, userId) {

var disabledNodes = [];

function traverse(state) {
// Check if this state is one that we are looking for
var n = datatree.get_node(state);
disabledNodes.push(n);

if (n.children) {
$.each(n.children, function(index, child) {
traverse(child);
});
}
datatree.disable_node(n);

}
var notOwned = false;
$.each(selected, function(index, node) {
// What types are being deleted and how many (for pluralization)
Expand All @@ -578,26 +588,38 @@ OME.handleDelete = function(deleteUrl, filesetCheckUrl, userId) {
// Disable the nodes marked for deletion
// Record them so they can easily be removed/re-enabled later
disabledNodes.push(node);
if (node.children) {
$.each(node.children, function(index, child) {
traverse(child);
});
}
datatree.disable_node(node);
});

// Hide warning, show it if e.g Image is in multiple groups
// https://forum.image.sc/t/caution-with-copy-links-in-omero/33680
$("#deleteCopyWarning").hide();
$("#delete-dialog-form").css('height', '92px');
OME.checkForMultipleParents(ajax_data, function(multiple){
if (multiple) {
$("#deleteCopyWarning").show();
$("#delete-dialog-form").css('height', '192px');
}
})

if (notOwned) {
$("#deleteOthersWarning").show();
} else {
$("#deleteOthersWarning").hide();
}

var type_strings = [];
var parent_types = {image:'dataset', dataset:'project', plate:'screen'};
var parent_strings = [];
for (var key in dtypes) {
if (parent_types[key]) {
parent_strings.push(parent_types[key].capitalize());
}
type_strings.push(key.replace("acquisition", "Run").capitalize() + (dtypes[key]>1 && "s" || ""));
}
var type_str = type_strings.join(" & "); // For delete dialog: E.g. 'Project & Datasets'
$("#delete_type").text(type_str);
$(".delete_type").text(type_str);
$(".delete_parent_type").text(parent_strings.join(" or "));
if (!askDeleteContents) $("#delete_contents_form").hide(); // don't ask about deleting contents

// callback when delete dialog is closed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@
WEBCLIENT.URLS.static_webgateway = "{% static 'webgateway' %}/";
WEBCLIENT.URLS.api_tags_and_tagged = "{% url 'api_tags_and_tagged' %}";
WEBCLIENT.URLS.fileset_check = "{% url 'fileset_check' 'delete' %}";
WEBCLIENT.URLS.api_parent_links = "{% url 'api_parent_links' %}";
WEBCLIENT.URLS.deletemany = "{% url 'manage_action_containers' 'deletemany' %}";
WEBCLIENT.URLS.copy_image_rdef_json = "{% url 'copy_image_rdef_json' %}";
WEBCLIENT.URLS.reset_owners_rdef_json = "{% url 'reset_owners_rdef_json' %}";
Expand Down
11 changes: 10 additions & 1 deletion omeroweb/webclient/templates/webclient/data/containers.html
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,16 @@
<p id="deleteOthersWarning" class='error' style="font-size: 120%; font-weight: bold">
Warning: Some objects you selected are owned by other users.
</p>
<p>Are you sure you want to delete the selected <span id="delete_type">Images</span>?</p>
<p id="deleteCopyWarning" class='error' style="font-size: 120%; font-weight: bold">
Warning: One or more <span class="delete_type">Images</span> are linked to multiple
<span class="delete_parent_type">Dataset</span>s.
This will DELETE them from ALL of those
<span class="delete_parent_type">Dataset</span>s.
If you only wish to remove <span class="delete_type">Images</span> from one
<span class="delete_parent_type">Dataset</span>, use the
"Cut Link" action.
</p>
<p>Are you sure you want to delete the selected <span class="delete_type">Images</span>?</p>
<p>If yes:</p>
<form>
<fieldset style="border: 0px solid white">
Expand Down
4 changes: 4 additions & 0 deletions omeroweb/webclient/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,10 @@
url(r'^api/paths_to_object/$', views.api_paths_to_object,
name='api_paths_to_object'),

# Get parents of 1 or more objects. ?image=1,2&dataset=3
url(r'^api/parent_links/$', views.api_parent_links,
name='api_parent_links'),

url(r'^api/tags/$', views.api_tags_and_tagged_list,
name='api_tags_and_tagged'),

Expand Down
35 changes: 35 additions & 0 deletions omeroweb/webclient/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,41 @@ def _api_links_DELETE(conn, json_data):
return JsonResponse(response)


@login_required()
def api_parent_links(request, conn=None, **kwargs):
"""
Get a list of links as
{'data': [{id: 12, child:{type:'image', id:1},
parent:{type:'dataset', id:2}] }

Supports ?image=1,2 and ?image=1&image=2
"""
parent_types = {'image': 'dataset',
'dataset': 'project',
'plate': 'screen'}
parents = []
for child_type, parent_type in parent_types.items():
ids = request.GET.getlist(child_type)
if len(ids) == 0:
continue
# support for ?image=1,2
child_ids = []
for id in ids:
for i in id.split(","):
child_ids.append(i)

link_type, result = get_object_links(conn, parent_type, None,
child_type, child_ids)
for link in result:
parents.append({
'id': link.id.val,
'parent': {'type': parent_type, 'id': link.parent.id.val},
'child': {'type': child_type, 'id': link.child.id.val}
})

return JsonResponse({'data': parents})


@login_required()
def api_paths_to_object(request, conn=None, **kwargs):
"""
Expand Down