-
Notifications
You must be signed in to change notification settings - Fork 1
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
2299 remove op rep #2443
2299 remove op rep #2443
Conversation
b5c8643
to
0153e9d
Compare
@@ -171,3 +171,73 @@ def test_register_operation_operation_representative_endpoint_bad_data(self): | |||
|
|||
# Assert | |||
assert response.status_code == 422 | |||
|
|||
|
|||
class TestOperationRepresentativePutEndpoint(CommonTestSetup): |
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.
🎵 all these tests 🥳 we don't need anymore 🎶 cause Sepehr did the beautiful permissions tests 🎵
33d7c8a
to
cd8efe2
Compare
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.
Minutiae that you shouldn't bother fixing in this PR because IMO it's entirely fine to leave these enhancements for post-MVP, but recording them here and now in hopes that someone else will remember these because I'll definitely forget:
- mousing over the garbage button shows as a pointer rather than a gloved hand as our other buttons do, so it's not as apparent that it's a button
- garbage can's background colour doesn't change on hover-over, whereas other buttons do
- I would include screenshots for these but Mac hides the pointer when you try to take a screenshot
Notes that are a bigger deal: we don't show the operation representatives in the Operation Details page. So the workflow for external users would go like this:
|
Operation reps are eventually supposed to show on the details page: https://www.figma.com/design/LsDmLDCdnJqI8UahyqvOD8/BCIERS-Designs?node-id=18025-1489&t=5B28WWF1rsdwkk9u-4. I think it's covered by this ticket: #2327 |
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.
looking good!
Apart from the merge conflict, just a reminder to update the endpoints_to_test
dictionary in the TestEndpointPermissions
class :)
items: { | ||
type: "string", | ||
enum: [1, 2], | ||
enumNames: ["Neville Flashdance", "Oz Twindlewinks"], |
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.
😂
@handle_http_errors() | ||
def remove_operation_representative( | ||
request: HttpRequest, operation_id: UUID, payload: OperationRepresentativeRemove | ||
) -> Tuple[Literal[200], None]: |
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.
@andrea-williams , is it fine to return None here? I don't think we actually need anything back. Even though it's None, what I actually get back is {'id': None}, not straight up None or {}
It's not a button, it's just an icon, so I'll make it a button and that will hopefully fix. Here's my opportunity to practice CSS... Edit: No CSS! MUI has a component for this: https://mui.com/material-ui/react-button/#icon-button |
cd8efe2
to
5a793f9
Compare
5a793f9
to
d4d8078
Compare
d4d8078
to
f578304
Compare
card: #2299
This PR:
I wasn't sure what to return from the endpoint since we don't need anything. I tried to return nothing (suggestions for something better?), but I'm getting {id: None}/{id: null} for some reason.