Police-Data-Accessibility-Project / data-sources-app

An API and UI for using and maintaining the Data Sources database
MIT License
2 stars 5 forks source link

Add `withdrawn` logic for `data_requests` and remove `volunteers_can_contact_requestor` #411

Open maxachis opened 3 weeks ago

maxachis commented 3 weeks ago

Context

Requirements

Tests

Docs

Open questions

josh-chamberlain commented 3 weeks ago

ah, so users shouldn't be able to modify the status directly—but I would love to get this done without adding a property. I think there are ways to do it without adding a new property + still keeping the API CRUDlike / not adding an endpoint: https://chatgpt.com/share/f7093f06-94f6-4f3a-9987-97636cde57e9

and, remember: chat GPT is never wrong. 😈

Anyway, I'm sure there are plenty of other ways to do this so I'm not too picky about it. I think this will be a common theme, though—lots of actions could potentially impact an item's status.

maxachis commented 3 weeks ago

@josh-chamberlain

After much thought and prayer over it, I can see some ways to do it while keeping it, er, CRUD-ish:

We can add a withdrawn parameter to the endpoint itself as a query parameter, which if true will ignore all other functionality and set it to Request withdrawn. We can even have it be reversible, which would move it back into Intake, perhaps.

Another option (though one I favor less) is to allow the user to specify a new value for Request status, but throw an error if it's modifying it to anything but Request withdrawn. They'll also need to spell it right and know that they can only do it for that functionality (while simultaneously ensuring that users with more admin-level roles know they can use other Request statuses; it'll probably get confusing).

What do you think?

josh-chamberlain commented 3 weeks ago

@maxachis hmm, I like the second option. In the UI, this would be on a straightforward path: the appropriate users could see a "withdraw request" button, and clicking it would hit the endpoint correctly (spelled right, checking permissions).

Even for API users—I see an UPDATE endpoint which accepts a status property. I make API calls to try to modify the status, and the app deals with me as it sees fit, sending a "you don't have permissions" message if necessary. We also have a wee table in the docs which might lead me to expect I cannot update the status. We could choose to make this reversible. It has the advantage of being a standard API design, which is user-friendly.

This is conceptually much simpler, and feels more like building a structure with 2x2 lego bricks rather than those super-special + unique pieces that aren't as transferable to other use cases. No matter what, we have to check to see if the user has the appropriate permissions—so we might as well structure things in a basic way.

josh-chamberlain commented 3 weeks ago

@maxachis After doing some soul-searching of my own, I think it's OK to have a whole endpoint with the specific function of updating a specific property, which hopefully makes permissions simpler. We shouldn't need to do too much of this, and can have broader endpoints for editing broader ranges of properties, but I think an endpoint called "withdraw-request" in the "requests" namespace is user-friendly and makes perfect sense! I was overthinking it before.

maxachis commented 2 weeks ago

@maxachis After doing some soul-searching of my own, I think it's OK to have a whole endpoint with the specific function of updating a specific property, which hopefully makes permissions simpler. We shouldn't need to do too much of this, and can have broader endpoints for editing broader ranges of properties, but I think an endpoint called "withdraw-request" in the "requests" namespace is user-friendly and makes perfect sense! I was overthinking it before.

Got it! Issue is updated!