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.

Sign up for GitHub

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

Jump to bottom

DSP-740 Update List Name #1727

Merged
merged 10 commits into from
Oct 9, 2020
Merged

DSP-740 Update List Name #1727

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0957d62
feature (updateListName) listName can be changed
SepidehAlassi Oct 6, 2020
687a015
fix (updateListName) add check for uniqueness of the names and fix th…
SepidehAlassi Oct 7, 2020
014fb89
refactor (updateListName) remove the redundant route
SepidehAlassi Oct 7, 2020
519b361
feature (updateList) make comments and labels in payload of update re…
SepidehAlassi Oct 7, 2020
b9fb92a
refactor (updateList) clear error message for empty labels or comment…
SepidehAlassi Oct 7, 2020
d65116a
Merge branch 'develop' into wip/DSP-740-UpdateListName
SepidehAlassi Oct 9, 2020
498bada
feature (updateListName) add test data for updating list name
SepidehAlassi Oct 9, 2020
675004b
refactor (updateListName) checks for empty optional labels and comments
SepidehAlassi Oct 9, 2020
a4db9d6
docs (updateListName) fix generate test data documentation
SepidehAlassi Oct 9, 2020
cba78e2
feature (updateListName) update `expected-client-test-data.txt`
SepidehAlassi Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 16 additions & 2 deletions docs/03-apis/api-admin/lists.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ License along with Knora. If not, see <http://www.gnu.org/licenses/>.
- `GET: /admin/lists[?projectIri=<projectIri>]` : return all lists optionally filtered by project
- `GET: /admin/lists/<listIri>` : return complete list with children
- `POST: /admin/lists` : create new list
- `PUT: /admin/lists/<listIri>` : update list information
- `POST: /admin/lists/<nodeIri>` : create new child node under the supplied parent node IRI
- NOT IMPLEMENTED: `DELETE: /admin/lists/<listIri>` : delete list including children if not used
- `GET: /admin/lists/infos/<listIri>` : return list information (without children)
- `PUT: /admin/lists/infos/<listIri>` : update list information

**List Node operations**

Expand Down Expand Up @@ -103,21 +103,35 @@ Additionally, each list can have an optional custom IRI (of [Knora IRI](../api-v
- GET: `/admin/lists/infos/<listIri>`

### Update list's information
The basic information of a list such as its labels, comments, name, or all of them can be updated. The parameters that
must be updated together with the new value must be given in the JSON body of the request together with the IRI of the
list and the IRI of the project it belongs to.

- Required permission: none
- Update list information
- PUT: `/admin/lists/infos/<listIri>`
- PUT: `/admin/lists/<listIri>`
- BODY:

```json
{
"listIri": "listIri",
"projectIri": "someprojectiri",
"name": "a new name",
"labels": [{ "value": "Neue geönderte Liste", "language": "de"}, { "value": "Changed list", "language": "en"}],
"comments": [{ "value": "Neuer Kommentar", "language": "de"}, { "value": "New comment", "language": "en"}]
}
```

If only name of the list must be updated, it can be given as below in the body of the request:

```json
{
"listIri": "listIri",
"projectIri": "someprojectiri",
"name": "another name"
}
```

## List Node Operations

### Get List Node Information
Expand Down
15 changes: 10 additions & 5 deletions ...main/scala/org/knora/webapi/messages/admin/responder/listsmessages/ListsMessagesADM.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,9 @@ case class CreateChildNodeApiRequestADM(parentNodeIri: IRI,
*/
case class ChangeListInfoApiRequestADM(listIri: IRI,
projectIri: IRI,
labels: Seq[StringLiteralV2],
comments: Seq[StringLiteralV2]) extends ListADMJsonProtocol {
name: Option[String] = None,
labels: Option[Seq[StringLiteralV2]] = None,
Copy link

Choose a reason for hiding this comment

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

If I understand correctly, the reason for the Option is that if you submit an empty list, it should remove all labels. Is that right? If so, could you add a test for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, if you submit labels=[] it will return a BedRequestException. At the time being, the idea is that the update route should not be used to delete the properties. This will change later.

Copy link

Choose a reason for hiding this comment

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

So why use Option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because without it, the user needs to repeat the labels and comments in the payload even if she wants to only change the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So when labels and comments are optional, user can simply use the following request body to update only the name of the list without having to give/repeat the values of labels and comments

   {
       "listIri": "listIri",
       "projectIri": "someprojectiri",
       "name": "another name"
  }

Copy link

Choose a reason for hiding this comment

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

OK, I understand, thanks.

comments: Option[Seq[StringLiteralV2]] = None) extends ListADMJsonProtocol {

private val stringFormatter = StringFormatter.getInstanceForConstantOntologies

Expand All @@ -150,10 +151,14 @@ case class ChangeListInfoApiRequestADM(listIri: IRI,
throw BadRequestException(PROJECT_IRI_INVALID_ERROR)
}

if (labels.isEmpty && comments.isEmpty) {
throw BadRequestException(REQUEST_NOT_CHANGING_DATA_ERROR)
// If payload containes label or comments they should not be empty
if (labels.nonEmpty && labels.get.isEmpty) {
SepidehAlassi marked this conversation as resolved.
Show resolved Hide resolved
throw BadRequestException(UPDATE_REQUEST_EMPTY_LABEL_OR_COMMENT_ERROR)
}

if (comments.nonEmpty && comments.get.isEmpty) {
SepidehAlassi marked this conversation as resolved.
Show resolved Hide resolved
throw BadRequestException(UPDATE_REQUEST_EMPTY_LABEL_OR_COMMENT_ERROR)
}
def toJsValue: JsValue = changeListInfoApiRequestADMFormat.write(this)
}

Expand Down Expand Up @@ -963,7 +968,7 @@ trait ListADMJsonProtocol extends SprayJsonSupport with DefaultJsonProtocol with

implicit val createListApiRequestADMFormat: RootJsonFormat[CreateListApiRequestADM] = jsonFormat(CreateListApiRequestADM, "id", "projectIri", "name", "labels", "comments")
implicit val createListNodeApiRequestADMFormat: RootJsonFormat[CreateChildNodeApiRequestADM] = jsonFormat(CreateChildNodeApiRequestADM, "parentNodeIri", "projectIri", "name", "labels", "comments")
implicit val changeListInfoApiRequestADMFormat: RootJsonFormat[ChangeListInfoApiRequestADM] = jsonFormat(ChangeListInfoApiRequestADM, "listIri", "projectIri", "labels", "comments")
implicit val changeListInfoApiRequestADMFormat: RootJsonFormat[ChangeListInfoApiRequestADM] = jsonFormat(ChangeListInfoApiRequestADM, "listIri", "projectIri", "name", "labels", "comments")
implicit val nodePathGetResponseADMFormat: RootJsonFormat[NodePathGetResponseADM] = jsonFormat(NodePathGetResponseADM, "elements")
implicit val listsGetResponseADMFormat: RootJsonFormat[ListsGetResponseADM] = jsonFormat(ListsGetResponseADM, "lists")
implicit val listGetResponseADMFormat: RootJsonFormat[ListGetResponseADM] = jsonFormat(ListGetResponseADM, "list")
Expand Down
2 changes: 1 addition & 1 deletion .../scala/org/knora/webapi/messages/admin/responder/listsmessages/ListsMessagesUtilADM.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ object ListsMessagesUtilADM {
val LABEL_MISSING_ERROR = "At least one label needs to be supplied."
val LIST_CREATE_PERMISSION_ERROR = "A list can only be created by the project or system administrator."
val LIST_CHANGE_PERMISSION_ERROR = "A list can only be changed by the project or system administrator."
val REQUEST_NOT_CHANGING_DATA_ERROR = "No data would be changed."
val UPDATE_REQUEST_EMPTY_LABEL_OR_COMMENT_ERROR = "List labels and comments cannot be empty."
}
29 changes: 21 additions & 8 deletions webapi/src/main/scala/org/knora/webapi/responders/admin/ListsResponderADM.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,18 +675,17 @@ class ListsResponderADM(responderData: ResponderData) extends Responder(responde
* The actual task run with an IRI lock.
*/
def listInfoChangeTask(listIri: IRI, changeListRequest: ChangeListInfoApiRequestADM, requestingUser: UserADM, apiRequestID: UUID): Future[ListInfoGetResponseADM] = for {
// check if required information is supplied
_ <- Future(if (changeListRequest.labels.isEmpty && changeListRequest.comments.isEmpty) throw BadRequestException(REQUEST_NOT_CHANGING_DATA_ERROR))
_ = if (!listIri.equals(changeListRequest.listIri)) throw BadRequestException("List IRI in path and payload don't match.")
// check if listIRI in path and payload match
_ <- Future(
if (!listIri.equals(changeListRequest.listIri)) throw BadRequestException("List IRI in path and payload don't match.")
)

// check if the requesting user is allowed to perform operation
_ <- Future(
if (!requestingUser.permissions.isProjectAdmin(changeListRequest.projectIri) && !requestingUser.permissions.isSystemAdmin) {
_ = if (!requestingUser.permissions.isProjectAdmin(changeListRequest.projectIri) && !requestingUser.permissions.isSystemAdmin) {
// not project or a system admin
// log.debug("same user: {}, system admin: {}", userProfile.userData.user_id.contains(userIri), userProfile.permissionData.isSystemAdmin)
throw ForbiddenException(LIST_CHANGE_PERMISSION_ERROR)
}
)

/* Verify that the list exists. */
maybeList <- listGetADM(rootNodeIri = listIri, requestingUser = KnoraSystemInstances.Users.SystemUser)
Expand All @@ -702,6 +701,14 @@ class ListsResponderADM(responderData: ResponderData) extends Responder(responde
case None => throw BadRequestException(s"Project '${list.listinfo.projectIri}' not found.")
}

/* verify that the list name is unique for the project */
nodeNameUnique: Boolean <- listNodeNameIsProjectUnique(changeListRequest.projectIri, changeListRequest.name)
_ = if (!nodeNameUnique) {
throw DuplicateValueException(s"The name ${changeListRequest.name.get} is already used by a list inside the project ${changeListRequest.projectIri}.")
}

hasOldName: Boolean = list.listinfo.name.nonEmpty

// get the data graph of the project.
dataNamedGraph = stringFormatter.projectDataNamedGraphV2(project)

Expand All @@ -710,6 +717,8 @@ class ListsResponderADM(responderData: ResponderData) extends Responder(responde
dataNamedGraph = dataNamedGraph,
triplestore = settings.triplestoreType,
listIri = listIri,
hasOldName = hasOldName,
maybeName = changeListRequest.name,
projectIri = project.id,
listClassIri = OntologyConstants.KnoraBase.ListNode,
maybeLabels = changeListRequest.labels,
Expand All @@ -725,15 +734,19 @@ class ListsResponderADM(responderData: ResponderData) extends Responder(responde


_ = if (changeListRequest.labels.nonEmpty) {
if (updatedList.listinfo.labels.stringLiterals.diff(changeListRequest.labels).nonEmpty) throw UpdateNotPerformedException("Lists's 'labels' where not updated. Please report this as a possible bug.")
if (updatedList.listinfo.labels.stringLiterals.diff(changeListRequest.labels.get).nonEmpty) throw UpdateNotPerformedException("Lists's 'labels' where not updated. Please report this as a possible bug.")
}

_ = if (changeListRequest.comments.nonEmpty) {
if (updatedList.listinfo.comments.stringLiterals.diff(changeListRequest.comments).nonEmpty)
if (updatedList.listinfo.comments.stringLiterals.diff(changeListRequest.comments.get).nonEmpty)

throw UpdateNotPerformedException("List's 'comments' was not updated. Please report this as a possible bug.")
}

_ = if (changeListRequest.name.nonEmpty) {
if (updatedList.listinfo.name.get != changeListRequest.name.get)
throw UpdateNotPerformedException("List's 'name' was not updated. Please report this as a possible bug.")
}
// _ = log.debug(s"listInfoChangeRequest - updatedList: {}", updatedList)

} yield ListInfoGetResponseADM(listinfo = updatedList.listinfo)
Expand Down
29 changes: 1 addition & 28 deletions webapi/src/main/scala/org/knora/webapi/routing/admin/ListsRouteADM.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class ListsRouteADM(routeData: KnoraRouteData) extends KnoraRoute(routeData) wit
* Returns the route.
*/
override def knoraApiPath: Route = getLists ~ postList ~ getList ~ putList ~ postListChildNode ~ deleteListNode ~
getListInfo ~ updateListInfo ~ getListNodeInfo
getListInfo ~ getListNodeInfo

/* return all lists optionally filtered by project */
@ApiOperation(value = "Get lists", nickname = "getlists", httpMethod = "GET", response = classOf[ListsGetResponseADM])
Expand Down Expand Up @@ -328,33 +328,6 @@ class ListsRouteADM(routeData: KnoraRouteData) extends KnoraRoute(routeData) wit
)
}

def updateListInfo: Route = path(ListsBasePath / "infos" / Segment) { iri =>
put {
/* update list info */
entity(as[ChangeListInfoApiRequestADM]) { apiRequest =>
requestContext =>
val listIri = stringFormatter.validateAndEscapeIri(iri, throw BadRequestException(s"Invalid param list IRI: $iri"))

val requestMessage: Future[ListInfoChangeRequestADM] = for {
requestingUser <- getUserADM(requestContext)
} yield ListInfoChangeRequestADM(
listIri = listIri,
changeListRequest = apiRequest,
requestingUser = requestingUser,
apiRequestID = UUID.randomUUID()
)

RouteUtilADM.runJsonRoute(
requestMessage,
requestContext,
settings,
responderManager,
log
)
}
}
}

def getListNodeInfo: Route = path(ListsBasePath / "nodes" / Segment) { iri =>
get {
/* return information about a single node (without children) */
Expand Down
22 changes: 18 additions & 4 deletions .../main/twirl/org/knora/webapi/messages/twirl/queries/sparql/admin/updateListInfo.scala.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
* @param listIri the IRI of the list we want to update.
* @param projectIri the IRI of the list's project.
* @param listClassIri the IRI of the OWL class that the list should belong to.
* @param hasOldName the old name of the list.
* @param maybeName the new name of the list.
* @param maybelabels the new optional label values.
* @param maybeComments the new optional comment values.
*@
Expand All @@ -37,8 +39,10 @@
listIri: IRI,
projectIri: IRI,
listClassIri: IRI,
maybeLabels: Seq[StringLiteralV2],
maybeComments: Seq[StringLiteralV2])
hasOldName: Boolean,
maybeName : Option[String],
maybeLabels: Option[Seq[StringLiteralV2]],
maybeComments: Option[Seq[StringLiteralV2]])

PREFIX rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#>
PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#>
Expand All @@ -54,6 +58,10 @@ DELETE {
?listIri rdfs:label ?currentLabels .
}

@if(hasOldName && maybeName.nonEmpty) {
?listIri knora-base:listNodeName ?currentListName .
}

@if(maybeComments.nonEmpty) {
?listIri rdfs:comment ?currentComments .
}
Expand All @@ -62,8 +70,12 @@ DELETE {

@* Add the new values. *@

@if(maybeName.nonEmpty) {
?listIri knora-base:listNodeName "@maybeName.get"^^xsd:string .
}

@if(maybeLabels.nonEmpty) {
@for(label <- maybeLabels) {
@for(label <- maybeLabels.get) {
@if(label.language.nonEmpty) {
?listIri rdfs:label """@label.value"""@@@{label.language.get} .
} else {
Expand All @@ -73,7 +85,7 @@ DELETE {
}

@if(maybeComments.nonEmpty) {
@for(comment <- maybeComments) {
@for(comment <- maybeComments.get) {
@if(comment.language.nonEmpty) {
?listIri rdfs:comment """@comment.value"""@@@{comment.language.get} .
} else {
Expand Down Expand Up @@ -109,6 +121,8 @@ WHERE {

?listIri knora-base:attachedToProject ?projectIri .

optional {?listIri knora-base:listNodeName ?currentListName .}

optional {?listIri rdfs:label ?currentLabels .}

optional {?listIri rdfs:comment ?currentComments .}
Expand Down
34 changes: 28 additions & 6 deletions webapi/src/test/scala/org/knora/webapi/e2e/admin/ListsADME2ESpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ class ListsADME2ESpec extends E2ESpec(ListsADME2ESpec.config) with SessionJsonPr
val params = SharedTestDataADM.updateListInfoRequest(newListIri.get)
val encodedListUrl = java.net.URLEncoder.encode(newListIri.get, "utf-8")

val request = Put(baseApiUrl + s"/admin/lists/infos/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val request = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val response: HttpResponse = singleAwaitingRequest(request)
// log.debug(s"response: ${response.toString}")
response.status should be(StatusCodes.OK)
Expand All @@ -315,11 +315,33 @@ class ListsADME2ESpec extends E2ESpec(ListsADME2ESpec.config) with SessionJsonPr
comments.size should be (2)
}

"update basic list information with a new name" in {
val params =
s"""{
| "listIri": "${newListIri.get}",
| "projectIri": "${SharedTestDataADM.ANYTHING_PROJECT_IRI}",
| "name": "a totally new name"
|}""".stripMargin

val encodedListUrl = java.net.URLEncoder.encode(newListIri.get, "utf-8")

val request = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val response: HttpResponse = singleAwaitingRequest(request)
// log.debug(s"response: ${response.toString}")
response.status should be(StatusCodes.OK)

val receivedListInfo: ListRootNodeInfoADM = AkkaHttpUtils.httpResponseToJson(response).fields("listinfo").convertTo[ListRootNodeInfoADM]

receivedListInfo.projectIri should be (SharedTestDataADM.ANYTHING_PROJECT_IRI)

receivedListInfo.name should be (Some("a totally new name"))
}

"update basic list information with repeated comment and label in different languages" in {
val params = SharedTestDataADM.updateListInfoWithRepeatedCommentAndLabelValuesRequest("http://rdfh.ch/lists/0001/treeList")
val encodedListUrl = java.net.URLEncoder.encode("http://rdfh.ch/lists/0001/treeList", "utf-8")

val request = Put(baseApiUrl + s"/admin/lists/infos/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val request = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val response: HttpResponse = singleAwaitingRequest(request)
// log.debug(s"response: ${response.toString}")
response.status should be(StatusCodes.OK)
Expand Down Expand Up @@ -348,7 +370,7 @@ class ListsADME2ESpec extends E2ESpec(ListsADME2ESpec.config) with SessionJsonPr

val encodedListUrl = java.net.URLEncoder.encode(newListIri.get, "utf-8")

val request = Put(baseApiUrl + s"/admin/lists/infos/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingUserCreds.basicHttpCredentials)
val request = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params)) ~> addCredentials(anythingUserCreds.basicHttpCredentials)
val response: HttpResponse = singleAwaitingRequest(request)
// log.debug(s"response: ${response.toString}")
response.status should be(StatusCodes.Forbidden)
Expand All @@ -369,7 +391,7 @@ class ListsADME2ESpec extends E2ESpec(ListsADME2ESpec.config) with SessionJsonPr
|}
""".stripMargin

val request01 = Put(baseApiUrl + s"/admin/lists/infos/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params01)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val request01 = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params01)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val response01: HttpResponse = singleAwaitingRequest(request01)
// log.debug(s"response: ${response.toString}")
response01.status should be(StatusCodes.BadRequest)
Expand All @@ -385,7 +407,7 @@ class ListsADME2ESpec extends E2ESpec(ListsADME2ESpec.config) with SessionJsonPr
|}
""".stripMargin

val request02 = Put(baseApiUrl + s"/admin/lists/infos/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params02)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val request02 = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params02)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val response02: HttpResponse = singleAwaitingRequest(request02)
// log.debug(s"response: ${response.toString}")
response02.status should be(StatusCodes.BadRequest)
Expand All @@ -401,7 +423,7 @@ class ListsADME2ESpec extends E2ESpec(ListsADME2ESpec.config) with SessionJsonPr
|}
""".stripMargin

val request03 = Put(baseApiUrl + s"/admin/lists/infos/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params03)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val request03 = Put(baseApiUrl + s"/admin/lists/" + encodedListUrl, HttpEntity(ContentTypes.`application/json`, params03)) ~> addCredentials(anythingAdminUserCreds.basicHttpCredentials)
val response03: HttpResponse = singleAwaitingRequest(request03)
// log.debug(s"response: ${response.toString}")
response03.status should be(StatusCodes.BadRequest)
Expand Down