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
Improve LTI: add create event and edit event, improve series tool #1289
Improve LTI: add create event and edit event, improve series tool #1289
Conversation
Bonus points for using the rarely used but fabulous word "embiggens" 👍 💯 |
74ee7d0
to
f4a0ae9
Compare
e6f8852
to
65bfe2e
Compare
88f12ee
to
8f9a78e
Compare
@@ -245,7 +254,7 @@ public Response getEpisodeAndSeriesById( | |||
name = "sign", type = RestParameter.Type.BOOLEAN) | |||
}, reponses = { @RestResponse(description = "The request was processed successfully.", responseCode = HttpServletResponse.SC_OK) }, returnDescription = "The search results, formatted as xml or json.") | |||
public Response getEpisode(@QueryParam("id") String id, @QueryParam("q") String text, | |||
@QueryParam("sid") String seriesId, @QueryParam("sort") String sort, @QueryParam("tag") String[] tags, @QueryParam("flavor") String[] flavors, | |||
@QueryParam("sid") String seriesId, @QueryParam("sname") String seriesName, @QueryParam("sort") String sort, @QueryParam("tag") String[] tags, @QueryParam("flavor") String[] flavors, | |||
@QueryParam("limit") int limit, @QueryParam("offset") int offset, @QueryParam("admin") boolean admin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That new sname param should to be added to the REST docs annotation describing the params for this method. It would be helpful to clarify in the REST doc annotation description that the used of "sname" param here assumes a unique series titles. i.e. not something like "Intro to Science 101" duplicated across different terms (like our site does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, fixed that one.
DublinCoreCatalogList result; | ||
try { | ||
result = seriesService.getSeries(new SeriesQuery().setSeriesTitle(seriesName)); | ||
} catch (SeriesException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past, we found it dangerous to tie the Search Service REST endpoint, that usually deploys to engage, to a service that usually lives on the Admin node. It caused the Engage player to be tightly dependent on the Admin node being up and accessible from the Engage node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like an out of date comment, but just in case: this is bound to the impl/remote, not the rest endpoint.
@@ -15,4 +15,6 @@ | |||
cardinality="1..1" policy="static" bind="setSearchService"/> | |||
<reference name="serviceregistry" interface="org.opencastproject.serviceregistry.api.ServiceRegistry" | |||
cardinality="1..1" policy="static" bind="setServiceRegistry"/> | |||
<reference name="seriesservice" interface="org.opencastproject.series.api.SeriesService" | |||
bind="setSeriesService"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REST endpoint assumes the series service exists and that this cardinality is 1..1. IMHO, it's human friendly to clarify that requirement in this doc.
HOWEVER, as I mentioned in early comment, tying the Engage Search service to the Admin's Series service, for sites that use the SeriesServiceRemote on Engage, can/will cause service interruptions for students watching lectures that was not expected. It would be more site friendly to make this dependency dynamic and optional and change the REST code to return an unimplemented 501, or similar if the sname is passed in.
NOTE that the seriesId param uses the Search Service series path. If a series has no events published to engage, the seriesId query returns the no-results response. It's odd for the sname to return different results than seriesId. The reason seriesId uses the Search Service is to prevent the possible binding of the endpoint on Engage (that students require to access and watch published lectures) to the Admin node's Series service impl.
Alternatively, the documentation for this can strongly recommend using external indices so no remote services are required (the service impls deployed on all remote nodes using the common external index and common external database).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thanks for this feedback "from the trenches". I hadn't thought of the problems of tying the two services together.
First, regarding the response in case the series doesn't exist. You're right, with a series ID instead of a name, this gives an empty result. I have now adapted the function to do that.
Second, regarding the dynamic dependency: I have now declared the series service dynamic
and of cardinality 0..1
. Also, the method returns 501 as you suggested (I thought about 503 instead, but couldn't make up my mind), so you don't pay a price if you're not using the feature. Since I'm on OSGi god, I'm hoping this is enough. The code seems to be working.
@@ -155,6 +205,9 @@ public WorkflowOperationResult start(final WorkflowInstance workflowInstance, fi | |||
final String configuredSourceFlavors = trimToEmpty(operation.getConfiguration(SOURCE_FLAVORS_PROPERTY)); | |||
final String configuredSourceTags = trimToEmpty(operation.getConfiguration(SOURCE_TAGS_PROPERTY)); | |||
final String configuredTargetTags = trimToEmpty(operation.getConfiguration(TARGET_TAGS_PROPERTY)); | |||
final boolean noSuffix = Boolean.parseBoolean(trimToEmpty(operation.getConfiguration(NO_SUFFIX))); | |||
final String seriesIdStr = operation.getConfiguration(SET_SERIES_ID); | |||
final String seriesId = seriesIdStr == null || seriesIdStr.startsWith("$") ? "" : seriesIdStr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why the set-series-id value in the operation configuration might start with a "$"? It seems like a really odd edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I remember this code making sense when I set up the new WOH, but I've now tested it without this code and it still works (I also removed parameters from the duplicate-event
workflow).
newMp = ingestService | ||
.addCatalog(new ByteArrayInputStream(series.dc.toXmlString().getBytes(StandardCharsets.UTF_8)), | ||
UUID.randomUUID().toString() + ".xml", MediaPackageElements.SERIES, newMp); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the new series ACL associated to this mediapackage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It...isn't. :D
Again, good catch. I added code to copy the series ACL now.
HI @pmiddend, what is the reason to keep the lti module intact and not to roll it into lti-service-api and lti-service-impl? Is the purpose of the lti-service modules to act as mediators between the lti module and Opencast services (i.e. job creation)? If so, would it have been possible for the lti module to have used the external API for executing jobs and services from Opencast? |
ba7ad46
to
e77fc64
Compare
With regards to the LTI Maven modules, you’re right, We could have merged I gave the External API some thought and in retrospect, I think we might have used it to build the LTI interface. However, we would have to have adapted it for the series names, and I’m not sure how the External API authentication plays along with LTI’s own mechanisms. |
e77fc64
to
b1b3620
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work. Nice to see the LTI-Tool with these features.
Some small hints.
Is it possible to configure the upload tracks. By default it is here in this tool presenter and caption.
The most common case i have seen is presenter only or presenter + presentation.
If you edit an Event the confirm-button should be save, or confirm. Not edit.
service.upsertEvent(null, captions, eventId, seriesId, metadataJson); | ||
return Response.ok().build(); | ||
} catch (FileUploadException | IOException e) { | ||
return Response.status(Status.INTERNAL_SERVER_ERROR).entity("error while uploading").build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uppercase
Error
@@ -232,6 +243,7 @@ public Response getEpisodeAndSeriesById( | |||
@RestParameter(description = "The ID of the single episode to be returned, if it exists.", isRequired = false, name = "id", type = RestParameter.Type.STRING), | |||
@RestParameter(description = "Any episode that matches this free-text query.", isRequired = false, name = "q", type = RestParameter.Type.STRING), | |||
@RestParameter(description = "Any episode that belongs to specified series id.", isRequired = false, name = "sid", type = RestParameter.Type.STRING), | |||
@RestParameter(description = "Any episode that belongs to specified series name (note that the specified series name must be unique).", isRequired = false, name = "sname", type = RestParameter.Type.STRING), | |||
// @RestParameter(defaultValue = "false", description = | |||
// "Whether to include this series episodes. This can be used in combination with \"id\" or \"q\".", | |||
// isRequired = false, name = "episodes", type = RestParameter.Type.STRING), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this uncommented? Maybe delete these 3 lines?
modules/lti/src/main/java/org/opencastproject/lti/LtiServlet.java
Outdated
Show resolved
Hide resolved
@CGreweling Thanks for your suggestions. I have changed the code accordingly. As to the the upload tracks, I realize this PR is pretty customer specific. You can only specify the presenter currently, not the presentation track. Ideally, you would have a kind of copy of the "New event" modal in LTI, with the possibility to hide certain fields. |
@pmiddend It's unintuitive to leave the "lti" module name as-is, while adding the lti-api, lti-impl, lti-remove module names. Does it reflect the "lti" module's new role if it was named "lti-endpoint"? (or, ugg, lti-external :p) [EDIT - just saw that the names are distinguished from lti as lti-service-XYZ!] lti-service-api, lti-service-impl, etc. |
@pmiddend Okay. I am fine with it as it is now. |
@CGreweling I've tested this quite often with multiple users and it "just works" apparently. |
I am wondering what the technical role of the lti module is at this point. If it is the external API to the internal LTI service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this pr.
86fc5a3
to
bdf978e
Compare
@pmiddend conflicts |
bdf978e
to
fb4e00d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things I found when scrolling through the code.
* @param seriesId ID of the series | ||
* @param metadataJson Metadata for the event as JSON string | ||
*/ | ||
void upsertEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? uploadEvent
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, the function inserts or updates, thus upserts. It's got a Wiktionary entry, even: https://en.wiktionary.org/wiki/upsert :)
@@ -0,0 +1,41 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use OSGi annotations instead?
@@ -0,0 +1,15 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Merged into the current
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't actually used the UI yet, this is just looking at the cod.e
...les/admin-ui-frontend/resources/public/org/opencastproject/adminui/languages/lang-en_US.json
Outdated
Show resolved
Hide resolved
@@ -15,6 +15,49 @@ | |||
"SELECT_NO_OPTION_SELECTED": "No option selected", | |||
"SELECT_NO_OPTIONS_AVAILABLE": "No options available", | |||
"YES": "Yes", | |||
"LTI": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rest of these translations avoided exclamation points, but I'm not sure. If so, please remove them from the new keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, you mean messages shouldn't end in a "!" in Opencast? Some apparently do in the lang-en_US.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then let's leave these as is.
modules/lti-service-impl/src/main/java/org/opencastproject/lti/service/impl/LtiServiceImpl.java
Outdated
Show resolved
Hide resolved
final MediaPackageElement captionsMpe = elementBuilder | ||
.newElement(MediaPackageElement.Type.Attachment, captionsFlavor); | ||
captionsMpe.setMimeType(mimeType("text", "vtt")); | ||
captionsMpe.addTag("lang:en"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these reasonable assumptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was basically the question I was asking myself as well. At the time of writing, the whole subtitle stuff was not very well documented, so I tried stuff out until paella/theodul actually displayed the subtitles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering Plapadoo's current state, I would be tempted to file this as an issue once this PR is in. This will come back and bite someone if we don't fix it at some point.
new AccessControlEntry("ROLE_OAUTH_USER", "write", true), | ||
new AccessControlEntry("ROLE_OAUTH_USER", "read", true)); | ||
this.authorizationService.setAcl(mp, AclScope.Episode, accessControlList); | ||
mp = ingestService.addTrack(file.getStream(), file.getSourceName(), MediaPackageElements.PRESENTER_SOURCE, mp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like you're adding the uploaded track as presenter/source
, with a hardcoded ACL. Both of those seem like things which ought to come in via parameters? Or am I completely out to lunch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the caption data questions, this should likely be filed as an issue as well.
modules/lti-service-impl/src/main/java/org/opencastproject/lti/service/impl/LtiServiceImpl.java
Show resolved
Hide resolved
.../lti-service-remote/src/main/java/org/opencastproject/lti/endpoint/LtiServiceRemoteImpl.java
Outdated
Show resolved
Hide resolved
.../lti-service-remote/src/main/java/org/opencastproject/lti/endpoint/LtiServiceRemoteImpl.java
Outdated
Show resolved
Hide resolved
DublinCoreCatalogList result; | ||
try { | ||
result = seriesService.getSeries(new SeriesQuery().setSeriesTitle(seriesName)); | ||
} catch (SeriesException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like an out of date comment, but just in case: this is bound to the impl/remote, not the rest endpoint.
@gregorydlogan I have hopefully fixed all capitalization issues! :) I've also fixed the build and pom issues. Note that I'm not working full-time on Opencast anymore, so there is way less time I can put into this PR. Sorry for that...Maybe I can look into the annotation stuff "later". |
@lkiesow I'm fine merging this as is, especially considering there are additional OSGI related changes which will need to go in. Thoughts? |
@pmiddend Conflicts, then I'll merge this in. |
@gregorydlogan I've contacted @pmiddend to see if he is still willing to fix this so we can finally see this merged into opencast. |
@pmiddend @flyapen Any news? Conflicts look minor to me. |
003fdec
to
11482c6
Compare
Hey guys, I hope you are all well. I am sorry - I know it took us ages to respond. We have all been busy working in our new jobs and moving to different cities. I just rebased Philipps branch and fixed the conflicts and compile problems. Unfortunately, I didn't have enough time to actually run Opencast and test if everything still works. |
@krinnewitz no worries. At this point we seemed satisfied with this, and the current Travis issue is a known issue in the service registry tests - I've restarted the build. I'm off from about now until Thursday, so assuming it works I would encourage @lkiesow (or someone he delegates) to merge it before more conflicts crop up. If not, I'll try to get to it Thursday. |
Thanks for the work guys. I've merge this in as is, if there's major breakage we'll have to chase it down. |
Abstract
This PR rewrites the whole LTI interface from mustache.js to ReactJS (using Typescript as the language of choice, instead of Javascript). It also embiggens LTI by adding/changing the following:
series
tool; this is optional and can be enabled via a tool parameterseries
tool; this is optional and can be enabled via a tool parameter.Uploading/Editing
The upload tool currently doesn’t really compare to the “New Event” modal in the admin UI. It was designer for a specific use case and is kept simple. It presents the user merely with the event title, a file input and some assorted metadata fields: presenter(s), license and language. Internally, a workflow can be configured which is executed after ingestion. Uploading a captions file is supported as well.
The upload tool also contains a list of running jobs, so the user gets some feedback about the upload completion and isn’t put off by the video not instantly appearing in the series tool.
The edit tool is almost the same, but it doesn’t have any upload features. It also has a “copy to different series” dropdown which triggers a new workflow (see below).
Deletion
Events can be deleted by pressing the trashcan symbol in the series list. To make this as simple as possible, this PR uses the “single step event deletion”, so the user doesn’t have to unpublish, wait, and then delete.
The Code
New modules
The
lti
module in Opencast is very minimalistic (which was sufficient for now). To support uploading and deletion, we had to write some Java code which needed services that are not necessarily available on the node LTI runs on. Our customer would like to run LTI on the presentation node, for example. To add multi-node support, we added the following modules and functions:lti-service-api
contains theLtiService
interface, implemented by the “remote” and the “impl”lti-service-impl
contains the implementationlti-service-remote
contains the remotelti
contains an additional endpoint which holds anLtiService
reference and serves as a “trampoline” to either the remote or the impl.While reviewing, do note that the endpoint in
lti
looks strikingly similar to the remote endpoint. That’s, unfortunately, unavoidable.The “copy to series” workflow
To implement the “copy to different series” functionality, we did three two things:
Translating LTI
Since we’re retrieving the event’s metadata list, which includes translation keys from the Admin UI, we though it’d be wise to simply copy the Admin UI’s translation files and add a new “LTI” section specifically for LTI translation keys. This, of course, means that LTI is sort of dependent on the Admin UI, but the copying seemed more “robust” than copy&pasting translations.
Attribution
This work was sponsored by Uni Gent.