-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add an endpoint to undeprecate files #4461
Add an endpoint to undeprecate files #4461
Conversation
Co-authored-by: Daniel Bell <[email protected]>
.undeprecate(fileId, rev) | ||
.index(mode) |
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 an issue of your PR but we should be doing indexing within the Files
here
"undeprecating a file" should { | ||
|
||
"succeed" in { | ||
givenADeprecatedFile { id => | ||
files.undeprecate(id, 2).accepted.deprecated shouldEqual false | ||
assertActive(id) | ||
} | ||
} | ||
|
||
"reject if file doesn't exists" in { | ||
files.undeprecate(fileId("404"), 1).rejectedWith[FileNotFound] | ||
} | ||
|
||
"reject if file is not deprecated" in { | ||
givenAFile { id => | ||
files.undeprecate(id, 1).assertRejectedWith[FileIsNotDeprecated] | ||
assertRemainsActive(id) | ||
} | ||
} | ||
|
||
"reject if the revision passed is incorrect" in { | ||
givenADeprecatedFile { id => | ||
files.undeprecate(id, 3).assertRejectedEquals(IncorrectRev(3, 2)) | ||
assertRemainsDeprecated(id) | ||
} | ||
} | ||
|
||
"reject if project does not exist" in { | ||
val wrongProject = ProjectRef(org, Label.unsafe("other")) | ||
files.deprecate(FileId(nxv + "id", wrongProject), 1).rejectedWith[ProjectContextRejection] | ||
} | ||
|
||
"reject if project is deprecated" in { | ||
files.undeprecate(FileId(nxv + "id", deprecatedProject.ref), 2).rejectedWith[ProjectContextRejection] | ||
} | ||
|
||
} | ||
|
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.
Really nice test
givenAFile { id => | ||
Put(s"/v1/files/org/proj/$id", entity()) ~> asWriter ~> routes ~> check { | ||
status shouldEqual StatusCodes.Conflict | ||
response.asJson shouldEqual jsonContentOf("/files/errors/already-exists.json", "id" -> (nxv + id)) |
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.
jsonContentOf("/files/errors/already-exists.json", "id" -> (nxv + id))
could become fileAlreadyExistsError(id)
?
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.
So I added the errors to the testkit but it has no dependency on the rdf module so it's not just fileAlreadyExistsError(id)
🤔
delta/testkit/src/main/scala/ch/epfl/bluebrain/nexus/testkit/scalatest/ce/CatsIOValues.scala
Outdated
Show resolved
Hide resolved
"reindex the previously deprecated file" in { | ||
givenADeprecatedFile { id => | ||
eventually { assertFileNotInListing(id) } | ||
undeprecateFile(id, rev = 2) | ||
eventually { assertFileIsInListing(id) } | ||
} | ||
} |
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 will be flaky
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.
But this test is also really nice
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.
Attempt to fix it
|
||
import ch.epfl.bluebrain.nexus.testkit.CirceLiteral.circeLiteralSyntax | ||
|
||
object FileErrors { |
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 feels a bit weird to have this here and the testkit being aware of a plugin.
Even if storage should not be a plugin imho.
And the remote container is in this module too
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.
Hum, yeah. I have merged this one but perhaps we can rework this. It could just be moved to the storage plugin
Closes #4402