#143 Added remove BR feature#176
Conversation
| const br = await BR.findOneAndDelete({ email: normalizedEmail }); | ||
| if (!br) return res.status(404).json({ error: "BR not found" }); | ||
|
|
||
| const user = await findUserByEmailInsensitive(normalizedEmail); |
There was a problem hiding this comment.
Right now, we are making two database trips here: one to read the user (findUserByEmailInsensitive) and one to write the change (user.save()). Since we only need to flip a boolean, we can optimize this into a single atomic database query using updateOne which reduces the network latency:
await User.updateOne(
{ email: normalizedEmail },
{ $set: { isBR: false } },
{ collation: { locale: "en", strength: 2 } }
);
RsbhThakur
left a comment
RsbhThakur
left a comment
There was a problem hiding this comment.
Nice Work!
While testing in local, I noticed that while adding a BR causes the frontend spinner to freeze for about 30-40 seconds. This happens because await fetchCoursesForBr(user.rollNumber) at line 29 and 68 in server/modules/br/br.controller.js scrapes the academic portal synchronously and blocks the HTTP response from returning.
Could you make this non-blocking by removing the await and adding a catch block? It will make the UI feel instant again while the courses fetch quietly in the background.
Suggested change in both createBR and updateBRs:
fetchCoursesForBr(user.rollNumber).catch((err) => logger.error(err));Tested in local by deleting the br - the br access was removed from the client, everything looks good to me, the pr is clean and self contained and working. just fix the two improvements as mentioned in this review.
2 participants
No description provided.