Skip to content
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

Merged
merged 8 commits into from
Nov 7, 2024
Merged

2299 remove op rep #2443

merged 8 commits into from
Nov 7, 2024

Conversation

BCerki
Copy link
Contributor

@BCerki BCerki commented Nov 5, 2024

card: #2299

This PR:

  • new widget for operation reps
  • vitests^
  • new endpoint to remove a rep
  • pytests^

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.

@@ -171,3 +171,73 @@ def test_register_operation_operation_representative_endpoint_bad_data(self):

# Assert
assert response.status_code == 422


class TestOperationRepresentativePutEndpoint(CommonTestSetup):
Copy link
Contributor Author

@BCerki BCerki Nov 6, 2024

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 🎵

@BCerki BCerki marked this pull request as ready for review November 6, 2024 22:38
Copy link
Contributor

@andrea-williams andrea-williams left a 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

@andrea-williams
Copy link
Contributor

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:

  1. go to Contacts tile, see "Places Assigned" for coworker Henry Ives who has now left the company. I see a note saying I must assign a different op rep for my registered operation before I can delete Henry Ives as a contact
  2. LOL I can't assign a different op rep because I can't get back to the registration multistep form now that the operation has been registered, and my operation reps don't appear in the Operation Details page
  3. Henry Ives must rescind his resignation letter

@BCerki
Copy link
Contributor Author

BCerki commented Nov 6, 2024

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:

  1. go to Contacts tile, see "Places Assigned" for coworker Henry Ives who has now left the company. I see a note saying I must assign a different op rep for my registered operation before I can delete Henry Ives as a contact
  2. LOL I can't assign a different op rep because I can't get back to the registration multistep form now that the operation has been registered, and my operation reps don't appear in the Operation Details page
  3. Henry Ives must rescind his resignation letter

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

Copy link
Contributor

@andrea-williams andrea-williams left a 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"],
Copy link
Contributor

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]:
Copy link
Contributor Author

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 {}

@BCerki
Copy link
Contributor Author

BCerki commented Nov 7, 2024

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

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

@BCerki BCerki merged commit f45df13 into develop Nov 7, 2024
40 checks passed
@BCerki BCerki deleted the 2299-remove-op-rep branch November 7, 2024 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants