-
Notifications
You must be signed in to change notification settings - Fork 30
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
Jatin get profile images from website #1116
base: development
Are you sure you want to change the base?
Jatin get profile images from website #1116
Conversation
Code work great, but I don't think it is necessary to introduce an extra dependency ( Asides from the code part, I wasn't quite sure why this functionality was needed since we already had the functionality to set avatar in the app. I've commented on the bugs docs for the same issue. |
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.
Reviewed the PR and provided my comments in Frontend PR. The code looks okay, but I am facing issues as mentioned in Frontend PR.
Removed puppeteer, using fetch to reduce load on the server. Please review again. |
dd0c2db
to
840e7b9
Compare
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.
Great work Jatin! Here are some of my suggested changes.
const getProfileImagesFromWebsite= async () => { | ||
try { | ||
// Fetch the webpage | ||
const response = await axios.get("https://www.onecommunityglobal.org/team"); |
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.
Consider adding timeout and retry logic. Without timeout and retry logic, if the network requests fail or hang, it could cause the application to become unresponsive.
return false; | ||
} | ||
|
||
async function imageUrlToPngBase64(url) { |
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.
Consider adding image size limit or image compression to ensure large size images won't slow down the server significantly.
}); | ||
var users=await userProfile.find({'isActive':true},"firstName lastName email profilePic suggestedProfilePics") | ||
|
||
users.map(async(u)=>{ |
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.
Your current implementation using map with async callbacks. I think this might lead to race conditions, since it doesn't wait for all promises to resolve. I would suggest using Promise.all to properly handle multiple asynchronous operations.
Description
Related PRS (if any):
To test this backend PR you need to checkout the #2714 frontend PR.
Main changes explained:
usercontroller.js
for removing and updating profile images.userhelper.js
for getting images from website.suggestedProfilePics
to store images from website.How to test:
npm install
and...
to run this PR locallynpm run build
andnpm start
.Screenshots or videos of changes:
profile_images.mp4