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

SAK-50724 Rubrics implement archive/merge #13085

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrianfish
Copy link
Contributor

@csev csev changed the title SAK-50724 rubric Implement archive/merge SAK-50724 rubric mplement archive/merge Dec 8, 2024
@csev csev changed the title SAK-50724 rubric mplement archive/merge SAK-50724 rubric implement archive/merge Dec 8, 2024
@csev csev changed the title SAK-50724 rubric implement archive/merge SAK-50724 Rubrics implement archive/merge Dec 8, 2024

RubricTransferBean rubricBean = new RubricTransferBean();
rubricBean.setOwnerId(toSiteId);
// TODO: Do we honour the original creator, or the current user?
Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for the creatorId - so far the only merge() that even looks at creatorId is Polls. The problem of using the original is if you are moving server to server - the user Id from the archive is meaningless and a possibly a security hole if it is used in an Authz role. I hope that the owner id is not an authz thing - and instead just some metadata. If it is just metadata - creatorId is correct (i.e. for the traditional Site Archive Import in a batch jobs) with will be admin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Owner id is the site where the rubric is hosted, ie: the siteId.

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 way the code is now the original creatorId is used if it is the user id of a valid Sakai user. If not, the current user, likely 'admin', is used.

rubricBean.setOwnerId(toSiteId);
// TODO: Do we honour the original creator, or the current user?
rubricBean.setCreatorId(creatorId);
// TODO: Do we honour the original created date, or use now?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think now. if we think precisely, it is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now it will be. We only need the original if we want some kind of audit trail

}

@Override
public String merge(String toSiteId, Element root, String archivePath, String fromSiteId, Map<String, String> attachmentNames, Map<String, String> userIdTrans, Set<String> userListAllowImport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I am thinking about is teaching these routines to handle multiple identical imports. They need to ignore the second and later imports of the same object. Canvas does this. For the site code, I did it by title. Before I started walking the element, I grabbed a list of the tools already in the site and then whilst I was looping through the element, just skip those that match. I can see issues with to skip or not to skip. But I think that de-duping is the better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, no problem. Makes sense .. but which one wins? What if one is newer and the user wants to use that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should probably align the ui with import from site, using the same append or replace content approach. Just line them up. Also, we should take cues from OS semantics. So "The thing you're replacing is newer that the thing you want to replace it with, do you really want to?"

@adrianfish adrianfish force-pushed the SAK-50724 branch 2 times, most recently from 49e3cfc to 00dbd96 Compare December 16, 2024 11:13
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