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

Updated the logic to return 404 instead of 200 when wrong taskRequest Id is sent #2296

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

Akshat187
Copy link

@Akshat187 Akshat187 commented Dec 14, 2024

Date: 14-12-24

Developer Name: akshat


Issue Ticket Number

(#2290 )

Description

When queried with taskRequestId which doesn't exist, fetchTaskRequestById was returning Tasks returned successfully instead of Task request not found, have changed the code to return the proper error message, currently there are no tests for this case, so it didn't get highlighted.

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Current implementation returning 200 OK, even when the id is incorrect image Updated implementation to return `Task request not found` when wrong id is passed image

Test Coverage

Screenshot 1

Changed the response returning Task Request not found when wrong taskRequestId is passed  · Real-Dev-Squad_website-backend@6cef89d - Google Chrome 20-12-2024 21_25_07

Additional Notes

Copy link
Contributor

@pankajjs pankajjs left a comment

Choose a reason for hiding this comment

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

Add screenshot

@AnujChhikara
Copy link

Can you please check if we have tets written for this, if yes then how are they passing before

@AnujChhikara
Copy link

Also can we please improve the title and PR description

@Akshat187 Akshat187 changed the title corrected the response of task request for given task request id Changed the response returning Task Request not found when wrong taskRequestId is passed. Dec 16, 2024
@prakashchoudhary07
Copy link
Contributor

Also can we please improve the title and PR description

+1

AnujChhikara

This comment was marked as resolved.

@vikasosmium
Copy link
Contributor

Update the API Contract and link it here for the error codes and other changes.

@vikasosmium
Copy link
Contributor

add tests SS

models/taskRequests.js Outdated Show resolved Hide resolved
@pankajjs
Copy link
Contributor

Add test coverage

Copy link
Contributor

@shubhdevelop shubhdevelop left a comment

Choose a reason for hiding this comment

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

catch block error message in the findTaskRequestById is not correct, can you please update it as well

@Akshat187
Copy link
Author

catch block error message in the findTaskRequestById is not correct, can you please update it as well

updated the error message

@Akshat187
Copy link
Author

Add test coverage

I have kept test coverage screenshot in the Description

@Akshat187
Copy link
Author

Real-Dev-Squad/website-api-contracts#207 API contract changes

@vikhyat187 vikhyat187 changed the title Changed the response returning Task Request not found when wrong taskRequestId is passed. Updated the logic to return 404 instead of 200 when wrong taskRequest Id is sent Dec 22, 2024
Copy link
Contributor

@pankajjs pankajjs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shubhdevelop shubhdevelop left a comment

Choose a reason for hiding this comment

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

please add unit test coverage for the fetchTaskRequestById()

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.

7 participants