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

TAA-136: MVP of the Approval Request and Approval Request List page functionality #576

Open
wants to merge 17 commits into
base: approvals-eap-main
Choose a base branch
from

Conversation

KlimekM
Copy link
Contributor

@KlimekM KlimekM commented Feb 4, 2025

Description

This PR implements the core Approval Request functionality for the Approval Request List Page and the individual Approval Request page.

The Approval Request List page allows filtering based on a given status, and searching by subject. The search functionality currently happens in the client. Clicking on an individual approval request links to the page for that approval request.

The individual approval request page allows a user to view the details of the approval request as well as approve/deny a request with comments.

Error handling follows the pattern set forth by Service Catalog and renders the same error view.

Screenshots

List Page Desktop:
Screenshot 2025-02-05 at 10 50 02 AM

List Page Mobile:
Screenshot 2025-02-05 at 10 59 06 AM

List Page functionality:
test-approval-request-page

Show Page Desktop:
Screenshot 2025-02-05 at 10 50 15 AM
Screenshot 2025-02-05 at 10 50 33 AM

Show Page Mobile:
Screenshot 2025-02-05 at 10 59 57 AM

Show Page Functionality:
test-approval

ErrorBoundary if request fails:
Screenshot 2025-02-05 at 11 09 08 AM

Checklist

  • 📗 all commit messages follow the conventional commits standard
  • ⬅️ changes are compatible with RTL direction
  • ♿ Changes to the UI are tested for accessibility and compliant with WCAG 2.1.
  • 📝 changes are tested in Chrome, Firefox, Safari and Edge
  • 📱 changes are responsive and tested in mobile
  • 👍 PR is approved by @zendesk/vikings

@KlimekM KlimekM requested review from a team as code owners February 4, 2025 22:48
}, [approvalRequests, searchTerm]);

if (error) {
throw error;

Choose a reason for hiding this comment

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

nit: i know this is still MVP but we could note to handle errors gracefully in the future like a growl or something with better UX

Choose a reason for hiding this comment

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

maybe similar to using notify below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout, I followed the pattern used in the Service Catalog: https://github.com/zendesk/copenhagen_theme/blob/beta/src/modules/service-catalog/ServiceCatalogItemPage.tsx#L49-L51

This relies on the ErrorBoundary in the src/modules/approval-requests/renderApprovalRequestList.tsx file which will display this standard error screen:
Screenshot 2025-02-05 at 11 09 08 AM

{/* <Option value="clarification_requested" label="Info needed" /> */}
<Option value="approved" label="Approved" />
<Option value="rejected" label="Denied" />
<Option value="withdrawn" label="Withdrawn" />

Choose a reason for hiding this comment

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

will these options always be hardcoded?

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 values will since they correspond with the accepted statuses in the API. The labels will be updated. I merged the translations earlier today and will follow up and update the text throughout

<MD>
{value.map((val) => (
<MultiselectTag key={val} hue="grey">
{val}

Choose a reason for hiding this comment

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

do we want to add support for nested dropdown values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oo, good call - let me try out a nested value to see what that would return as since we are using a custom endpoint for these.

return <MD>{value ? "Yes" : "No"}</MD>;
}

return <MD>{value}</MD>;

Choose a reason for hiding this comment

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

will this never be a date or lurf value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be a LURF or a Date or any other custom field that is exposed to customers but in the instance of a LURF/Date the API returns a string value:
Screenshot 2025-02-05 at 11 19 42 AM

const notify = useNotify();
const [comment, setComment] = useState("");
const [pendingStatus, setPendingStatus] = useState<
"APPROVED" | "REJECTED" | null

Choose a reason for hiding this comment

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

nit: these strings can likely be stored in CONSTs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll update

@KlimekM KlimekM force-pushed the KlimekM/approvals-eap-1 branch from 8c2e45b to c386be0 Compare February 5, 2025 17:00
@KlimekM KlimekM force-pushed the KlimekM/approvals-eap-1 branch from c386be0 to c44cbc5 Compare February 5, 2025 17:38
@SirishaNeduri
Copy link

@KlimekM The YML file is empty so there is no translation review needed. please let us know if the file has strings to be reviewed.

@KlimekM
Copy link
Contributor Author

KlimekM commented Feb 5, 2025

@KlimekM The YML file is empty so there is no translation review needed. please let us know if the file has strings to be reviewed.

Hey @SirishaNeduri, sorry about that - I put the translations up in a separate PR: #575 that @kubraokcu reviewed and merged this morning to kick off the translation process and will update here to use those strings as the content when they are available

@@ -66,7 +68,8 @@
"@svgr/rollup": "^8.1.0",
"@testing-library/dom": "^9.3.1",
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/user-event": "^14.4.3",
"@testing-library/react": "^12.1.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only using testing-library/react version 12 here because that is what is compatible with react 17 which is the current version of react within the repo

@KlimekM KlimekM force-pushed the KlimekM/approvals-eap-1 branch from e6be3c7 to 0c77487 Compare February 5, 2025 23:03
{ "src/**/": "KEBAB_CASE" },
{
"src/**/!(__tests__)/": "KEBAB_CASE",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luis-almeida I ended up adding a __tests__ directory within the approval-requests module and this was throwing an eslint error on those files. Let me know if you want to handle this differently.

@kubraokcu kubraokcu removed the request for review from a team February 6, 2025 09:36
@KlimekM KlimekM force-pushed the KlimekM/approvals-eap-1 branch from 56823b1 to 9ae1976 Compare February 11, 2025 18:09
@@ -3,6 +3,7 @@
"target": "ESNext",
"lib": ["DOM", "DOM.Iterable", "ESNext"],
"allowJs": false,
"esModuleInterop": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here to account for lodash.debounce import error in test files

More context: https://www.typescriptlang.org/tsconfig/#esModuleInterop

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.

3 participants