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

feat: return a DeletedResource or DeletedValue instead of 404 if a deleted resource or value is requested (DEV-226) #1960

Merged
merged 35 commits into from Jan 5, 2022

Conversation

@BalduinLandolt
Copy link
Contributor

@BalduinLandolt BalduinLandolt commented Dec 7, 2021

resolves DEV-226

@BalduinLandolt BalduinLandolt changed the title feat: return a DeletedResource instead of 404 if a deleted resource is requested (DEV-226) feat: return a DeletedResource or DeletedValue instead of 404 if a deleted resource or value is requested (DEV-226) Dec 9, 2021
@BalduinLandolt BalduinLandolt marked this pull request as ready for review Dec 17, 2021
@BalduinLandolt BalduinLandolt requested a review from subotic as a code owner Dec 17, 2021
Copy link
Contributor

@irinaschubert irinaschubert left a comment

LGTM, just some minor suggesions. I have some questions, I would like to discuss, so I didn't approve yet.

I don't know where to put this so I just write it here: In resources we have creationDate and deleteDate. Shouldn't it be deletionDate to be more consistent? For values, we have valueCreationDate and deleteDate. Shouldn't it be valueDeletionDate?

And, shouldn't we log the user who deleted a resource/value as well? As far as I could see there is only the deletion date and the comment available (but maybe I misunderstood the code). I would do so. As a digital archive we should be able to say who did what.

I couldn't really review the jsonld, ttl, and rdf files as there were too many changes...

@@ -220,56 +219,66 @@ class ValuesResponderV2Spec extends CoreSpec() with ImplicitSender {
private def checkValueIsDeleted(
resourceIri: IRI,
maybePreviousLastModDate: Option[Instant],
propertyIriForGravsearch: SmartIri,
propertyIriInResult: SmartIri,
valueIri: IRI,
customDeleteDate: Option[Instant] = None,
Copy link
Contributor

@irinaschubert irinaschubert Dec 20, 2021

Choose a reason for hiding this comment

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

I would call it deleteDate instead of customDeleteDate (I see that it wasn't you who named it like that, but still).

docs/03-apis/api-v2/editing-resources.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/editing-resources.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/editing-resources.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/editing-resources.md Show resolved Hide resolved
docs/03-apis/api-v2/editing-resources.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/editing-resources.md Outdated Show resolved Hide resolved
docs/03-apis/api-v2/editing-values.md Outdated Show resolved Hide resolved
"@value": "http://0.0.0.0:3336/ark:/72163/1/0001/a=thingO/sWSymIzAS_qXqyHLhwbwwAU.20211216T18193124797Z",
"@type": "xsd:anyURI"
},
"knora-api:userHasPermission": "RV",
Copy link
Contributor

@mpro7 mpro7 Jan 3, 2022

Choose a reason for hiding this comment

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

Suggested change
"knora-api:userHasPermission": "RV",
"knora-api:userPermission": "RV",

"@id": "http://rdfh.ch/users/9XBCrDV3SRa7kS1WwynB4Q"
},
"knora-api:valueHasUUID": "sWSymIzAS_qXqyHLhwbwwA",
"knora-api:hasPermissions": "CR knora-admin:Creator|M knora-admin:ProjectMember|V knora-admin:KnownUser|RV knora-admin:UnknownUser",
Copy link
Contributor

@mpro7 mpro7 Jan 3, 2022

Choose a reason for hiding this comment

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

Suggested change
"knora-api:hasPermissions": "CR knora-admin:Creator|M knora-admin:ProjectMember|V knora-admin:KnownUser|RV knora-admin:UnknownUser",
"knora-api:permissions": "CR knora-admin:Creator|M knora-admin:ProjectMember|V knora-admin:KnownUser|RV knora-admin:UnknownUser",

or with relevant prefix

@sonarcloud
Copy link

@sonarcloud sonarcloud bot commented Jan 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
9.1% 9.1% Duplication

mpro7
mpro7 approved these changes Jan 3, 2022
subotic
subotic approved these changes Jan 5, 2022
@BalduinLandolt BalduinLandolt merged commit c78e252 into main Jan 5, 2022
13 checks passed
@BalduinLandolt BalduinLandolt deleted the wip/DEV-226-show-deleted-resources branch Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants