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

Task #229290 [WEB] Remove the submissionId from the payload of the confirm API #2254

Open
wants to merge 1 commit into
base: release-2.8.1
Choose a base branch
from

Conversation

nawaz-ss
Copy link
Contributor

@nawaz-ss nawaz-ss commented Oct 25, 2024

https://tracker.tekdi.net/issues/229290

I have ensured that following Pull Request Checklist is taken care of before sending this PR

  • Code is formatted as per format decided
  • Updated acceptance criteria in tracker
  • Updated test cases in test-cases-tracker

Summary by CodeRabbit

  • New Features

    • Improved error handling in the Job Details component for better user feedback.
    • Adjustments to the submission ID management in the Automated Form component.
  • Bug Fixes

    • Enhanced rendering logic with unique keys to prevent rendering issues.
  • Chores

    • Renamed variables for consistency and clarity in the Job Details component.

…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
Copy link

coderabbitai bot commented Oct 25, 2024

Walkthrough

The changes involve modifications to two components: JobDetails.jsx and AutomatedForm.jsx. In JobDetails.jsx, variable naming conventions were improved, and error handling in data fetching was enhanced. The handling of the siteUrl state was removed, and initial data fetching logic was commented out. In AutomatedForm.jsx, the management of submission IDs was altered, with key lines commented out, impacting how submission IDs are tracked and stored. Overall, the changes focus on error handling, naming consistency, and adjustments to data management practices.

Changes

File Path Change Summary
apps/front-end/src/onest/job/JobDetails.jsx Renamed settransactionId to setTransactionId. Removed handling of siteUrl. Improved error handling in fetchJobDetails. Commented out fetchData call in useEffect. Adjusted rendering logic with unique keys.
apps/front-end/src/onest/scholarship/AutomatedForm.jsx Commented out lines for managing submission_id in request body and local storage, indicating a shift in submission ID handling. Error handling and loading states remain unchanged.

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
Loading
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
Loading

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
9.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link

@coderabbitai coderabbitai bot left a 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:

  1. Use React state or context for managing form submission state
  2. 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

📥 Commits

Files that changed from the base of the PR and between e82bb3e and ecf8714.

📒 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 convention

The rename from settransactionId to setTransactionId follows React's camelCase convention for state setter functions.


260-265: Verify removal of tracking functionality

The 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 + ")",
Copy link

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}>
Copy link

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

Comment on lines +184 to +194
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

Copy link

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();
Copy link

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:

  1. Retrieving user's application data for the specific job
  2. Setting application list data via setListData
  3. 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

Comment on lines +192 to +194
// initReqBodyJson.init[1].message.order.items[0].xinput.form[
// "submission_id"
// ] = localStorage.getItem("submissionId");
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

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", "")
  • 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

@Pritams43 Pritams43 changed the base branch from release-2.7.7 to release-2.8.0 November 4, 2024 05:58
@Pritams43 Pritams43 changed the base branch from release-2.8.0 to release-2.8.1 November 21, 2024 06:19
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.

1 participant