admin: deleted records link with include_deleted parameter#3368
admin: deleted records link with include_deleted parameter#3368ptamarit wants to merge 1 commit into
Conversation
slint
left a comment
slint
left a comment
There was a problem hiding this comment.
LGTM, just a tiny nit, not critical for merging.
|
|
||
| let selfLink = result.links.self_html; | ||
| if (result?.deletion_status?.is_deleted) { | ||
| selfLink += "?include_deleted=1"; |
There was a problem hiding this comment.
nit: could we "formally" build the URL, by parsing and setting the param? This is more of a future-proofing thing, in case at some point we already bake into the URL a different querystring param
There was a problem hiding this comment.
Good point, done. Now using new URL and searchParams.set("include_deleted", "1").
ptamarit
force-pushed
the
admin-deleted-records-include-deleted
branch
from
bed5276 to
e14c2fe
Compare
| const recordOwner = result?.parent?.access?.owned_by; | ||
|
|
||
| let selfLink = result.links.self_html; | ||
| if (result?.deletion_status?.is_deleted) { |
There was a problem hiding this comment.
question: why do we do that in the component and not in the backend?
There was a problem hiding this comment.
I don't think it makes sense to expose the "include_deleted" links in the API response, since only admins have access to it, and it's only needed for the admin panel.
There was a problem hiding this comment.
my concerns are that we mix responsibilities and that could introduce problems in the future. as of now i thought the only place where links are created is in the links section in the service layer.
There was a problem hiding this comment.
We would need a conditional self_html_deleted link that also takes into account the user's identity... Which I think we have access to from ctx["identity"]. It could work tbh, and it would be more backend-driven indeed.
Though another PR in invenio-rdm-records 😬
|
This PR was automatically marked as stale. |
3 participants
When reviewing deleted records, administrators usually want to see the deleted records content and not the tombstones.
It probably does not make sense to expose the "include_deleted" links in the API response, so we built the URL client-side (only needed for the admin panel).