Skip to content
This repository was archived by the owner on Aug 7, 2024. It is now read-only.

fix: improve error handling with HTTP utility functions #8283

Closed
wants to merge 17 commits into from

Conversation

badrivlog
Copy link
Contributor

@badrivlog badrivlog commented Jul 20, 2023

Fixes Issue

Closes #8112

What?

added a new util file apiRequests.js, containing functions for sending API requests.
Implemented sendRequest function that handles various types of API requests, handles rejected promise and also converts the response to JSON

Why?

abstract api request functionality into one function that we can use all over our project.
These changes improved error handling

How?

replace fetch with sendRequest in manage profile sections

Testing?

Step to test

  • Pull down this branch
  • visit localhost:3000/account/manage/profile (link, milestone, event, testimonials)
  • send an invalid api request by sendRequest function
  • see the request actually rejected and displayed the error to UI

Anything else?

Let's consider implementing it other areas of the project. I will happy to doing that work💪

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

Implemented a sendRequest utility functions to handle
various types of HTTP requests, these functions provide a
convenient interface for making API request and interacting with
server resources.
updated:   pages/account/manage/profile.js
@Pradumnasaraf Pradumnasaraf added 💻 aspect: code undefined issue linked Pull Request has issue linked labels Jul 22, 2023
Refactored manage profile sections of the codebase to utilize the new
sendRequest utility function for making api requests.
@badrivlog badrivlog marked this pull request as ready for review July 28, 2023 10:21
replace fetch with sendRequest, handling error and display error to ui
@badrivlog
Copy link
Contributor Author

Hey, could I have some update regarding this PR?

@eddiejaoude
Copy link
Member

Hey, could I have some update regarding this PR?

Sorry for the delay, I have shared with the maintainers and I am waiting to hear their thoughts on it

@badrivlog badrivlog requested a review from azhechkova August 9, 2023 18:11
@badrivlog
Copy link
Contributor Author

@azhechkova,

I'd greatly appreciate it if you could take another look at the pull request and let me know if the changes align with your expectations. Your expertise is truly valued in ensuring the quality of the code.

Thank you for your time and consideration!

@azhechkova
Copy link
Contributor

@badrivlog sorry for the delay, PR looks good!

@badrivlog
Copy link
Contributor Author

badrivlog commented Aug 11, 2023

@badrivlog sorry for the delay, PR looks good!

No problem at all! Thanks for taking the time to review👍

Copy link
Member

@lukeecart lukeecart left a comment

Choose a reason for hiding this comment

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

I like the idea and left a few comments

@badrivlog
Copy link
Contributor Author

I like the idea and left a few comments

Thanks for taking the time to review👍

@badrivlog
Copy link
Contributor Author

@lukeecart,

Addressed the suggested changes and made updates to the PR👍 Could you please take another look and let me know if everything aligns with your expectations? Your feedback is important to me

Copy link
Member

@lukeecart lukeecart left a comment

Choose a reason for hiding this comment

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

Lgtm. Nice job

@badrivlog
Copy link
Contributor Author

@eddiejaoude, I believe we're now ready to proceed with merging the PR into the main codebase. Could you please take a look at the changes and, if everything looks good to you, give the final approval for merging?

@eddiejaoude
Copy link
Member

@eddiejaoude, I believe we're now ready to proceed with merging the PR into the main codebase. Could you please take a look at the changes and, if everything looks good to you, give the final approval for merging?

Thank you @badrivlog for your efforts on this, but after discussing with the other maintainers, we feel it it extra abstraction that is not required, so I will close for now. Sorry for letting this go so far but I was trying to understand the benefit.

@badrivlog
Copy link
Contributor Author

@eddiejaoude,Thank you🙏 for your prompt attention to my PR. I appreciate the time you took to review it and consider its potential impact. While I'm disappointed that the changes didn't align with the project's current direction, I completely understand and respect your decision💯

I'm looking forward to contributing further to the project in a way that aligns with its direction👍

@badrivlog badrivlog deleted the issue-8112 branch August 15, 2023 17:47
@eddiejaoude
Copy link
Member

Thank you for understanding @badrivlog - yes please continue contributing to LinkFree, I look forward to geeking out more 💪

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💻 aspect: code undefined issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] needs proper error handling in manage form section
5 participants