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

2059 internal user approvals #2551

Merged
merged 6 commits into from
Dec 13, 2024
Merged

2059 internal user approvals #2551

merged 6 commits into from
Dec 13, 2024

Conversation

BCerki
Copy link
Contributor

@BCerki BCerki commented Dec 5, 2024

card: https://github.com/orgs/bcgov/projects/123/views/16?pane=issue&itemId=80445965&issue=bcgov%7Ccas-registration%7C2059

This PR:

  • copies over components from reg1, some are tweaked
  • vitests the tweaks (if the reviewer feels we need more testing, happy to add; I skipped the components that I brought over from reg1 and didn't change)
  • new, more generic accordion component (the reg1 one uses FormBase, and the nested reg2 forms didn't play nice with it)
  • refactor of the operator schemas to make them more reusable (fetch the dropdown options in a function rather than the component)
  • new endpoint to fetch all the info about an operator (previously we were using the one for fetching the data on the confirm-you've-selected-the-right-operator page, which only has partial info)
  • update mock data (the test CAS user is a director)
  • test with Existing Operator 6 Legal Name, which is already approved (or make a new operator). Some of the test operators are not approved (because in reg1 e2e tests, we test the approval process), so if you try with one of those, you'll get a backend error if you try to approve an admin for a non-approved operator.
  • Happo is flake
  • I've manually tested the select operator workflow and admin operator page (since I changed the API), as well as the new feature to approve/decline

@BCerki BCerki force-pushed the 2059-internal-user-approvals branch from 3003e30 to 156ffbe Compare December 5, 2024 23:58
@BCerki BCerki changed the title chore: spec work 2059 internal user approvals Dec 6, 2024
@BCerki BCerki force-pushed the 2059-internal-user-approvals branch from 156ffbe to 818f9ac Compare December 6, 2024 17:55
@@ -139,3 +139,22 @@ def test_list_user_operators_v2():
)
assert user_operators_sorted_by_status.first().status == UserOperator.Statuses.APPROVED
assert user_operators_sorted_by_status.last().status == UserOperator.Statuses.PENDING

@staticmethod
def test_create_operator_and_user_operator_v2():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do a full test for this because I didn't want to get bogged down in tech debt; I just tested the bits relevant to the AC of this card

@BCerki BCerki force-pushed the 2059-internal-user-approvals branch 2 times, most recently from 5d114f0 to 0f105e2 Compare December 9, 2024 17:48
@@ -16,7 +17,7 @@

@router.get(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the Administration > Operators > Operator ID page; it wasn't fetching all the info

@BCerki BCerki force-pushed the 2059-internal-user-approvals branch 4 times, most recently from 2a162cd to 384b4d1 Compare December 11, 2024 18:52
@BCerki BCerki marked this pull request as ready for review December 11, 2024 19:08
Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🤩 Just left a couple of comments/nits but nothing too major!
I only noticed we have the cancel button under the operator form and not at the bottom of the page:
Screenshot 2024-12-12 at 2 17 48 PM

operator = baker.make_recipe('utils.operator')
mock_get_operator_confirm.return_value = operator
# Act: Mock the authorization and perform the request
TestUtils.authorize_current_user_as_operator_user(self, operator=operator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we don't need this line.

@@ -25,6 +25,7 @@ def test_get_current_operator_and_user_operator(self):
assert "municipality" in response_json
assert "province" in response_json
assert "postal_code" in response_json
# brianna
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover!

Comment on lines +7 to +35
const ChangeIcon = () => (
<svg
width="16"
height="16"
viewBox="0 0 16 16"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M14.9367 4.67668L13.5861 6.02732C13.4484 6.16502 13.2257 6.16502 13.088 6.02732L9.83593 2.77524C9.69823 2.63754 9.69823 2.41487 9.83593 2.27717L11.1866 0.92653C11.7345 0.378657 12.6251 0.378657 13.1759 0.92653L14.9367 2.68734C15.4876 3.23522 15.4876 4.12588 14.9367 4.67668ZM8.67572 3.43737L0.982001 11.131L0.360879 14.6907C0.275914 15.1712 0.694878 15.5873 1.17537 15.5052L4.7351 14.8812L12.4288 7.18752C12.5665 7.04982 12.5665 6.82716 12.4288 6.68945L9.17672 3.43737C9.03609 3.29967 8.81342 3.29967 8.67572 3.43737ZM3.98507 10.4718C3.82393 10.3107 3.82393 10.0529 3.98507 9.89173L8.497 5.37983C8.65814 5.21869 8.91596 5.21869 9.0771 5.37983C9.23824 5.54097 9.23824 5.79879 9.0771 5.95993L4.56517 10.4718C4.40403 10.633 4.14621 10.633 3.98507 10.4718ZM2.9274 12.9358H4.33372V13.9993L2.44398 14.3304L1.53281 13.4192L1.86388 11.5295H2.9274V12.9358Z"
fill="white"
/>
</svg>
);

const TimeIcon = () => (
<svg
width="15"
height="15"
viewBox="0 0 15 15"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M7.26562 0.25C3.25195 0.25 0 3.50195 0 7.51562C0 11.5293 3.25195 14.7812 7.26562 14.7812C11.2793 14.7812 14.5312 11.5293 14.5312 7.51562C14.5312 3.50195 11.2793 0.25 7.26562 0.25ZM8.93848 10.5068L6.35449 8.62891C6.26367 8.56152 6.21094 8.45605 6.21094 8.34473V3.41406C6.21094 3.2207 6.36914 3.0625 6.5625 3.0625H7.96875C8.16211 3.0625 8.32031 3.2207 8.32031 3.41406V7.44824L10.1807 8.80176C10.3389 8.91602 10.3711 9.13574 10.2568 9.29395L9.43066 10.4307C9.31641 10.5859 9.09668 10.6211 8.93848 10.5068Z"
fill="#313132"
/>
</svg>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we move these icons under lib/components/src/icons, right next to the others?

name: string;
}

import { OperatorStatus } from "@bciers/utils/src/enums";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: imports at the top!
Also, this file has some unused types; should we remove them?

}
});

it("renders the approve/decline buttons when user is cas_director or cas_analyast", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo cas_analyast

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userEvent.... is an async function; we need to add await to ensure they are working properly.

Comment on lines 33 to 35
const [rerenderKey, setRerenderKey] = useState(
crypto.getRandomValues(new Uint32Array(1))[0],
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rerenderKey is being used, setRerenderKey isn't

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original implementation, it was used to force a re-render of the form to update its state. However, in this case, we don't need it because we are not forcing a re-render.

@BCerki BCerki force-pushed the 2059-internal-user-approvals branch 2 times, most recently from 6af4dc8 to 1b34b67 Compare December 13, 2024 17:57
@BCerki BCerki force-pushed the 2059-internal-user-approvals branch 2 times, most recently from a2e9c34 to 2b43732 Compare December 13, 2024 18:38
@BCerki BCerki merged commit aae6450 into develop Dec 13, 2024
42 checks passed
@BCerki BCerki deleted the 2059-internal-user-approvals branch December 13, 2024 18:55
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