SAK-50724 Rubrics implement archive/merge #13085
Conversation
csev
changed the title
csev
changed the title
csev
changed the title
|
|
||
| RubricTransferBean rubricBean = new RubricTransferBean(); | ||
| rubricBean.setOwnerId(toSiteId); | ||
| // TODO: Do we honour the original creator, or the current user? |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Owner id is the site where the rubric is hosted, ie: the siteId.
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
I think now. if we think precisely, it is now.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
force-pushed
the
SAK-50724
branch
2 times, most recently
from
49e3cfc to
00dbd96
Compare
adrianfish
force-pushed
the
SAK-50724
branch
from
00dbd96 to
1cf6f96
Compare
|
LGTM - Thanks for the ignore duplicates code. |
| import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; | ||
| import com.fasterxml.jackson.datatype.jsr310.ser.InstantSerializer; | ||
|
|
||
| public class MapperFactory { |
There was a problem hiding this comment.
Just a note that ObjectMapper and XmlMapper are thread safe and can be reused
There was a problem hiding this comment.
Also I think that it is a good idea to extend the Repository to provide serialization of the data objects it manages.
There was a problem hiding this comment.
note I think this class fine for setting up the mappers and configuring them though I think it would be better to follow the builder pattern
There was a problem hiding this comment.
ObjectMapper objectMapper = new MapperBuilder()
.withSerializationFeature(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
.withDeserializationFeature(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
.json(); // or .xml();
There was a problem hiding this comment.
that is just some sample psuedo code.
Totally like the idea of updating BasicSerializationRepsoitory with the new builder I would advocate for something similar for the Rubrics and Conversations Repositories
There was a problem hiding this comment.
I'm not sure I want to add toJSON and toXML methods to the SpringCrudRepository interface. Spring itself doesn't mix and match like that and I wanted to keep the SpringCrudRepository stuff reasonably similar to Spring's own to make it an easier switch to Spring proxied implementations of our interfaces. I like the mapper factory approach because it centralises that piece, standardises it across Sakai, but I'm not sure about going the whole hog and adding toXML and toJSON.
There was a problem hiding this comment.
How do you add serializer and deserializer properties to the mapper builder's factory though? I can't see how you configure the Woodstox stuff in the builder pattern.
| @@ -33,6 +33,7 @@ | |||
| <value>WebService</value> | |||
| <value>LessonBuilderEntityProducer</value> | |||
| <value>PollListManager</value> | |||
| <value>rubrics</value> | |||
There was a problem hiding this comment.
shouldn't this match what is in default.sakai.properties?
There was a problem hiding this comment.
It matches what getLabel returns in RubricsServiceImpl
| try { | ||
| return rubricsService.getRubricsForSite(siteId).stream().map(b -> entityModelForRubricBean(b)).collect(Collectors.toList()); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
https://sakaiproject.atlassian.net/browse/SAK-50724