-
Notifications
You must be signed in to change notification settings - Fork 27
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
Task #229290 [WEB] Remove the submissionId from the payload of the confirm API #2254
base: release-2.8.1
Are you sure you want to change the base?
Conversation
…ONDC Sandbox Flow || Task #228808 [WEB UI] Test the Jobs flow on Sandbox with the ONDC network || Task #229290 [WEB] Remove the submissionId from the payload of the confirm API
WalkthroughThe changes involve modifications to two components: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JobDetails
participant API
User->>JobDetails: Request Job Details
JobDetails->>API: Fetch Job Details
API-->>JobDetails: Return Job Data
JobDetails->>User: Display Job Details
sequenceDiagram
participant User
participant AutomatedForm
participant API
User->>AutomatedForm: Submit Form
AutomatedForm->>API: Send Request (without submission_id)
API-->>AutomatedForm: Confirm Submission
AutomatedForm->>User: Display Confirmation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
Quality Gate failedFailed conditions |
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.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
apps/front-end/src/onest/scholarship/AutomatedForm.jsx (1)
490-493
: Consider using React state instead of localStorage.The current implementation heavily relies on localStorage for state management, which can lead to race conditions and make the code harder to test.
Consider these improvements:
- Use React state or context for managing form submission state
- If persistence is needed, consider using a more appropriate solution like:
- Redux persist
- React Query with persistence
- Custom hooks with controlled persistence
This will make the code more maintainable, testable, and less prone to race conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/front-end/src/onest/job/JobDetails.jsx (8 hunks)
- apps/front-end/src/onest/scholarship/AutomatedForm.jsx (2 hunks)
🔇 Additional comments (3)
apps/front-end/src/onest/job/JobDetails.jsx (2)
33-33
: LGTM: Improved naming conventionThe rename from
settransactionId
tosetTransactionId
follows React's camelCase convention for state setter functions.
260-265
: Verify removal of tracking functionalityThe removal of telemetry and analytics tracking should be verified to ensure it's intentional and won't impact business requirements.
apps/front-end/src/onest/scholarship/AutomatedForm.jsx (1)
Line range hint
485-503
: Verify the form submission flow after removing submissionId.The form submission logic needs to be verified as it previously relied on submissionId for flow control.
Let's analyze the form submission flow:
#!/bin/bash # Search for form submission handlers and related functions ast-grep --pattern 'submitFormDetail($$$)' -l js,jsx ast-grep --pattern 'confirmDetails($$$)' -l js,jsx # Search for any error handling related to form submission rg -A 5 "Error submitting form"
if (data?.responses[0]?.message?.order?.items[0]) { | ||
setJobsData(data?.responses[0]?.message?.order?.items[0]); | ||
} else { | ||
errorMessage( | ||
t("Delay_in_fetching_the_details") + "(" + transactionId + ")" | ||
t("Delay_in_fetching_the_details") + "(" + transactionId + ")", |
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.
🛠️ Refactor suggestion
Consider user-friendly error messages
While including the transaction ID is helpful for debugging, consider showing a more user-friendly message to end users while logging the technical details.
-errorMessage(t("Delay_in_fetching_the_details") + "(" + transactionId + ")");
+errorMessage(t("Delay_in_fetching_the_details"));
+console.error("Failed to fetch details for transaction:", transactionId);
Also applies to: 210-210
@@ -453,12 +429,12 @@ function JobDetails() { | |||
)} | |||
<Box marginTop={4}> | |||
{jobsData?.tags?.map((tag, index) => ( | |||
<Box key={index} marginBottom={3}> | |||
<Box key={index + tag} marginBottom={3}> |
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.
🛠️ Refactor suggestion
Consider using unique identifiers for React keys
While adding keys improves rendering, using array indices in keys is not recommended as it can lead to rendering issues if the list order changes. Consider using unique identifiers from the tag and item objects if available.
-<Box key={index + tag} marginBottom={3}>
+<Box key={tag.descriptor?.name || index} marginBottom={3}>
-<div key={itemIndex + item}>
+<div key={item.descriptor?.code || item.descriptor?.name || itemIndex}>
Also applies to: 437-437
if (!response.ok) { | ||
// Handle non-200 responses | ||
throw new Error(`HTTP error! status: ${response.status}`); | ||
} | ||
const responseText = await response.text(); // Get response as text first | ||
if (!responseText) { | ||
throw new Error("Response is empty"); | ||
} | ||
|
||
const data = JSON.parse(responseText); // Now parse the JSON | ||
|
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.
🛠️ Refactor suggestion
Good error handling improvements, but consider user feedback
The added checks for HTTP status and empty responses improve robustness. However, these errors should be communicated to the user via the existing errorMessage
toast function.
if (!response.ok) {
- throw new Error(`HTTP error! status: ${response.status}`);
+ errorMessage(t("Failed_to_fetch_job_details"));
+ throw new Error(`HTTP error! status: ${response.status}`);
}
const responseText = await response.text();
if (!responseText) {
- throw new Error("Response is empty");
+ errorMessage(t("No_job_details_available"));
+ throw new Error("Response is empty");
}
Committable suggestion was skipped due to low confidence.
@@ -117,7 +114,7 @@ function JobDetails() { | |||
getApplicationStatus(result?.data[0].order_id, result?.data[0]?.id); | |||
} | |||
}; | |||
fetchData(); | |||
//fetchData(); |
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.
💡 Codebase verification
Commenting out fetchData() impacts application tracking functionality
The commented out fetchData()
function is responsible for:
- Retrieving user's application data for the specific job
- Setting application list data via
setListData
- Triggering application status tracking via
getApplicationStatus
Without this function:
- The application status won't be fetched and tracked
- The UI won't show the application status/success modal
- The user won't be able to see their previous application for this job
🔗 Analysis chain
Verify impact of disabled data fetching
The commented out fetchData()
call affects application status tracking. Please verify if this is intentional and aligns with the PR objective of removing the submissionId.
Let me search for the implementation of the commented out fetchData()
function to understand its purpose.
Let me check if there are any dependencies or state variables that are affected by this fetchData function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of fetchData or related application status tracking
rg -A 5 "getApplicationStatus|fetchData|updateApplicationStatus"
Length of output: 151503
Script:
#!/bin/bash
# Search for fetchData implementation in JobDetails.jsx
rg -A 10 "const fetchData = async" apps/front-end/src/onest/job/JobDetails.jsx
Length of output: 486
Script:
#!/bin/bash
# Search for listData usage and related state in JobDetails.jsx
rg -B 5 -A 10 "setListData|listData" apps/front-end/src/onest/job/JobDetails.jsx
Length of output: 1503
// initReqBodyJson.init[1].message.order.items[0].xinput.form[ | ||
// "submission_id" | ||
// ] = localStorage.getItem("submissionId"); |
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.
💡 Codebase verification
Inconsistent removal of submissionId requires attention
The verification reveals that submissionId is still actively used in multiple places:
AutomatedForm.jsx
:- Checking submissionId in conditional logic:
if (localStorage.getItem("submissionId"))
- Clearing submissionId:
localStorage.setItem("submissionId", "")
- Checking submissionId in conditional logic:
MediaPage.jsx
:- Setting submissionId in localStorage
- Using submissionId in form submission
- Multiple references in payload construction
These findings indicate that the removal of submissionId from the confirm API payload is incomplete. The changes need to be coordinated across all related components to maintain consistency.
🔗 Analysis chain
Ensure consistent removal of submissionId.
While the submissionId has been removed from the confirm API payload, there are still references to it in the codebase that need to be addressed.
Consider removing or updating these related code segments:
- if (localStorage.getItem("submissionId")) {
+ // Update the condition based on the new flow
+ if (/* new condition */) {
Let's verify all references to submissionId in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to submissionId
rg -i "submissionId"
Length of output: 5116
Script:
#!/bin/bash
# Search for any remaining references to submissionId in the codebase
rg -i "submissionid" -A 3 -B 3
Length of output: 5064
https://tracker.tekdi.net/issues/229290
I have ensured that following
Pull Request Checklist
is taken care of before sending this PRSummary by CodeRabbit
New Features
Bug Fixes
Chores