feat: Add onboarding extension requests tab #942
Conversation
AnujChhikara
force-pushed
the
feature/onboarding-extension-tab
branch
from
9cc17c7 to
87e445e
Compare
AnujChhikara
force-pushed
the
feature/onboarding-extension-tab
branch
from
87e445e to
9840e76
Compare
| expect(await actionContainer.isVisible()).toBe(false); | ||
| expect(await approveButton.isVisible()).toBe(false); | ||
| expect(await rejectButton.isVisible()).toBe(false); | ||
| expect(await remarkInput.isVisible()).toBe(false); |
There was a problem hiding this comment.
I used isVisible() because all elements (e.g., action container, buttons, input) are in the DOM but hidden with a CSS class (like hidden). inDocument only checks DOM presence, and toBeTrue doesn’t accurately verify visibility. isVisible() ensures we test if elements are actually visible on the page or not.
| --color-error: #da1e28; | ||
| --color-warn: rgba(199, 129, 18, 0.4); | ||
| --color-warn: #e3a008; |
There was a problem hiding this comment.
these hex codes are taken from the design doc.
| .request_container > * { | ||
| width: 100% !important; | ||
| } | ||
|
|
There was a problem hiding this comment.
I added width: 100% !important to .request_container > * for request cards in dev mode, where we use a flex row layout (flex-direction: column). This ensures cards stretch to full width for consistency. In production (grid layout), width: auto works better to avoid cramped spacing, but dev=true requires width: 100% for a clean appearance.
AnujChhikara
changed the title
pankajjs
changed the title
| @@ -24,6 +26,7 @@ const ErrorMessages = { | |||
| UNAUTHORIZED: 'You are unauthorized to view this section', | |||
| OOO_NOT_FOUND: 'OOO Requests not found', | |||
| EXTENSION_NOT_FOUND: 'Extension Requests not found', | |||
| ONBOARDING_EXTENSION_NOT_FOUND: 'Onboarding extension Requests not found', | |||
There was a problem hiding this comment.
I can't find this key getting used anywhere, was this intended to be used in requests/script.js line number 120?
There was a problem hiding this comment.
This will be used when we apply the filter. As for its use on line 120, we don’t display that error directly. If you check the renderRequests function, it wraps the entire API call in a try-catch block and only shows an error in the finally block if the request response is undefined or zero. Here’s the link
WalkthroughThe pull request introduces onboarding extension request handling into the codebase. It adds new mock data and constants, updates test cases to verify the onboarding tab’s behavior, and integrates this functionality into the main request pipeline. In addition, several CSS files have been edited to update styling and improve layout while certain utility functions have been refactored for enhanced testability. The changes consolidate request fetching into a single function that now processes multiple request types including the new onboarding type. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant RequestHandler
participant API
User->>UI: Click "Onboarding" Tab
UI->>RequestHandler: Set request type to ONBOARDING and update tab state
RequestHandler->>API: Call getRequests(ONBOARDING)
API-->>RequestHandler: Return onboarding extension data (mocked)
RequestHandler->>UI: Render updated request cards
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
requests/style.css (3)
137-137: Introducing amax-width: 46remconstraint.
This may help keep request cards from stretching excessively, though consider responsive breakpoints for smaller screens if further refinement is needed.
239-239: Switching.request__date__pillcolor to--black-color.
The black text color on a pill highlights date details more clearly than white.
369-369: Slight margin adjustment for.request__remark__input.
margin-bottom: 0.2rem;offers subtle spacing and helps separate the input from subsequent elements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
__tests__/requests/requests.test.js(4 hunks)global.css(0 hunks)mock-data/requests/index.js(1 hunks)requests/constants.js(2 hunks)requests/index.html(2 hunks)requests/script.js(20 hunks)requests/style.css(6 hunks)requests/util.js(2 hunks)
💤 Files with no reviewable changes (1)
- global.css
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (37)
mock-data/requests/index.js (1)
76-110: Looks good - new mock data matches existing patternsThe mock data for onboarding extension requests follows the same structure as the existing request types, including appropriate properties and sample states for testing.
requests/constants.js (3)
10-10: LGTM - new request type constantThe onboarding extension request type constant follows the same naming convention as the existing request types.
14-14: LGTM - new tab ID constantThe constant for the onboarding extension tab ID follows the same naming pattern as the existing tab IDs.
29-29: LGTM - new error messageThe error message for onboarding extension requests follows the same format as existing error messages.
requests/util.js (2)
27-28: Good addition for testabilityAdding support for data-testid attributes enhances the testability of the components and aligns with good testing practices.
37-47:❓ Verification inconclusive
Good refactoring to consolidate query parameter logic
The new
getQueryParamsStringfunction efficiently replaces the need for separate functions for each request type, improving code maintainability.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the removed function # Search for any remaining references to getOooQueryParamsString rg "getOooQueryParamsString" -A 3 -B 3Length of output: 38
Refactoring Approved – Please Manually Verify Removal of
getOooQueryParamsStringThe new
getQueryParamsStringfunction appropriately consolidates the query parameter logic. Although our automated search forgetOooQueryParamsStringreturned no output, this result is somewhat inconclusive. Please manually verify that there are no remaining references to the removedgetOooQueryParamsStringfunction throughout the codebase.
- File to Review:
requests/util.js(Lines 37–47)- Action: Confirm manually that no parts of the code are still invoking
getOooQueryParamsString.__tests__/requests/requests.test.js (5)
7-7: LGTM - import of new mock dataThe import of the onboarding extension request mock data follows the same pattern as existing imports.
70-82: Good test coverage for new request typeThe test intercepts the API request for onboarding extension requests and responds with the appropriate mock data, following the same pattern as the existing request types.
131-134: Improved test reliability with waitForFunctionUsing
waitForFunctionto check for the updated text content is more reliable than just waiting for a selector, as it ensures the content has actually changed.
161-164: Good test for feature flag behaviorThis test verifies that the onboarding tab is hidden when the dev flag is not set, which is important for ensuring the feature flag works correctly.
166-268: Comprehensive test coverage for onboarding requestsThe tests properly verify all aspects of the onboarding extension feature:
- Tab visibility in dev mode
- Request rendering
- Action buttons visibility based on request state
- Superuser details visibility based on request state
The tests are well-structured and thorough.
requests/index.html (2)
37-64: Good addition ofdata-testidattributes for testing.
These data attributes enhance test clarity by enabling selectors that are explicitly tied to functional elements like OOO, Onboarding, Task, and Extension tabs. The use ofclass="disabled__tab hidden"for "Onboarding" ensures it remains hidden unless toggled, which aligns with the dev-based feature flag.
72-76:data-testidfor request container is consistent with testing best practices.
Adding a unique test identifier on the principal container simplifies test queries and fosters reliable UI tests.requests/style.css (7)
4-6: Switching to solid hex colors benefits consistency.
Changing--color-successand--color-warnto solid hex codes (#0e9f6e, #e3a008) improves visual predictability and aligns them with typical design standards.
116-126: New.request_containerrules enhance vertical flow for dev usage.
Defining a custom flex-direction and forcing child elements to full width ensures a vertical stacking layout needed in development mode. However, confirm that toggling between.requestand.request_containerdoes not inadvertently break other layouts.
133-133: Renaming.ooo_request__cardto.request__card(observed) makes the style more generic.
A generic name suits multiple request types. Ensure all references to.ooo_request__cardhave been removed or updated throughout the codebase.
180-181: Increasing font size and weight for.requester__name p.
Enhancing legibility from 0.7rem+300 to 0.8rem+500 is a solid improvement in readability.
232-232: Raising.request__timeline pfont weight for a bolder emphasis.
The shift to 500 helps the timeline text stand out, improving scanability.
295-295: Matching.admin__name__and__remark pfont weight to 500.
Keeping font weights consistent across different text sections promotes a uniform look.
365-365: Boosting.request__remark__inputpadding to0.8rem.
Additional padding can improve the user’s typing experience, especially on mobile screens.requests/script.js (17)
12-14: Retrieving the onboarding extension tab element.
Consider adding safeguards ifONBOARDING_EXTENSION_TAB_IDis missing or undefined to prevent potential runtime errors.
27-31: Conditional dev-mode setup ensures onboarding tab is only visible in development.
Switching from.requestto.request_containerchanges layout from grid to flex. Confirm it doesn’t break existing styling beyond dev usage.
53-64: Event listener for OOO tab is consistent with new approach.
The addition ofpreventDefault()and asynchronous rendering logic aligns with the existing flow.
66-77: Extension tab click handler matches the updated pattern.
Ensuring uniform handling across all tabs improves maintainability.
79-90: Onboarding tab event listener introduced.
Mirroring the logic of OOO and extension tabs ensures a cohesive user flow for the new request type.
98-105: UnifiedgetRequestsfunction logic for multiple request types.
Consolidating into one function is simpler to maintain and properly addresses the onboarding scenario withONBOARDING_EXTENSION_NOT_FOUND.
123-123: Displaying the appropriate 404 error message.
Switching the shown message based on request type ensures better user feedback in the onboarding flow.
164-165: Destructuring new fields for onboarding.
IncludingnewEndsOnandoldEndsOnensures you have easy access to the extended date properties within request objects.
174-174: FlaggingisRequestTypeOnboardingonce for clarity.
This boolean check helps streamline the subsequent logic for distinguishing date fields and messages.
194-201: Conditional date fields based on request type.
ApplyingoldEndsOn/newEndsOnfor onboarding vs.from/untilfor OOO is precise. Well done ensuring minimal confusion in the code.
214-215: AddingtestIdto the card container.
testId: \${type.toLowerCase()}-request-card`` helps testers confirm which card type is rendered.
229-229: IntroducingtestId: 'action-container'.
Clear test identifiers make verifying the presence/absence of action controls more straightforward in E2E and integration tests.
234-234:testId: 'request-remark-input'clarifies input field usage.
This ensures remark input is easy to target in automated tests.
246-246:testId: 'approve-button'for acceptance flow.
Explicit labeling for acceptance logic helps reliably locate the element in UI tests.
259-259:testId: 'reject-button'for rejection flow.
Similar to approve, it’s valuable to keep distinct element identifiers for consistent test automation.
279-279:testId: 'admin-info-and-status'fosters improved monitoring of superuser details.
This simplifies verifying whether admin info is shown or hidden.
449-450: LoadingrequesterUserDetailsfromrequest.userIdfor non-OOO requests.
This distinction ensures OOO requests userequestedBywhile onboarding and extension requests rely onuserId. Check if any other request types might need a similar fallback or additional mapping.
Date: 24 Feb, 2025
Developer Name: @AnujChhikara
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
brave_W1zSAH4AtF.mp4
Test Coverage
Screenshot 1
Additional Notes
Summary by CodeRabbit
New Features
Style