[API] Recycle bin endpoints - #3377
Conversation
bourdaisj
changed the title
|
Hi @Julesdevops, |
9dd12c5 to
782291b
Compare
782291b to
f14e6e8
Compare
|
I think the three endpoints I already implemented are fine as-is. |
|
@Julesdevops I would prefer the most minimal approach that covers your use-case, which sounds like it may be covered by the existing three? If (and only if) the extra detail was really needed I'd be tempted to say an additional parameter on the existing book/chapter/page/shelf "read" endpoints might be better suited to avoid needing a more complex implementation for this specific use-case. |
|
@Julesdevops Let me know your intention either way. No rush on the actual work but I'll be wrapping up the next feature release over the next week so, if you're happy with the current state, I can plan to sneak this in |
|
@ssddanbrown It already covers my use-case indeed. If someone is facing a more complex use-case someday I'd be happy to follow-up on this though. |
ea66904 to
5307cdb
Compare
5307cdb to
14bccae
Compare
Made the following changes, many of these are just to align with existing conventions. - Updated urls to be hypenated, instead of underscored, to match other system endpoints. - Updated URL parameter to be `deletionId` instead of `id`, and removed the ID-based comment on controller methods, so the required ID model is clear from the URL alone, since its not clear from the URL endpoint alone like existing endpoints. This follows the pattern used in the "web" routes. - Added extra detail on some controller method comments, and copied permission comment to each method. - Removed existing field visibility mechanisms to use simpler model-based visibility since we didn't need anything too special here (After some of my other changes). - Allowed the "deletable" model to be shown in response to provide a little more detail on the main deleted item. - Updated parent/child-count loading to be on the "deletable" model instead of additional properties which results in simpler controller logic and enforces the idea these are relations on the deletable, not the deletion itself. It also removes additional exposure of model namespacing. - Updated (int) casts to intval, just since that's our most common conversion method in the codebase. - Testing: Removed `actingAsAuthorizedUser` and used the admin user instead to prevent extra auth steps on each test. - Testing: Cut logic/data-checks from tests if already covered by other tests. - Testing: Added simple assertions for delete/restore response data. - Examples: Updated list example to reflect changes. Review of PR #3377 To be followed up with changes to polymorphic relations to hide namespacing.
|
Thanks again for this @Julesdevops, Now merged for next release. During review, I did make a series of changes detailed in commit ff8dade. Everything was functionally great and high-quality, most of the changes I made were just alignment with existing codebase conventions. The main significant other change was around how entity children and parent details were provided back on the list response. If you have any queries on my changes feel free to ask. I wasn't a fan of exposing internal namespacing for API use ( |
|
@ssddanbrown Fair enough. Thanks for following up on this so quickly! |
2 participants
Related to: #3372
This is a draft PR to discuss and make sure I'm on the right track.