-
Notifications
You must be signed in to change notification settings - Fork 263
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
Reading Schema Update #193
Reading Schema Update #193
Conversation
updating changes to reading model fix updated reading model
@ArpitKotecha please review his task! |
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.
@lakshyajit165 Waiting for @ArpitKotecha to confirm if this is not duplicate.
@@ -6,7 +6,23 @@ const readingSchema = new mongoose.Schema({ | |||
value: { | |||
required: true, | |||
type: String | |||
}, | |||
tankId: { | |||
required: true, |
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.
@ArpitKotecha Can you please review this. I see similar PR at other places too? Can you please verify.
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.
@vinitshahdeo Probably while creating/modifying routes in other PR's they have modified the schema accordingly.
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.
@ArpitKotecha Can you please review this. I see similar PR at other places too? Can you please verify.
@vinitshahdeo Maybe we can merge this and request in other PR to include this schema instead of modifying by themselve.
@vinitshahdeo, if this's a duplicate then how come was I assigned to it in the first place? Am I not supposed to get any points for this then? |
@lakshyajit165 Nope - I'm not talking about the duplicate issue, I'm talking about the duplicate code. For sure, you'll get the marks. We don't want you to lose marks. Take a chill pill.
|
@vinitshahdeo I respect what you say and fully agree with you. It's just that, I want it to get reviewed so that I could, take up other issues or suggest new ones if I am allowed. |
@lakshyajit165 Sure, please allow us some time. Our mentors will review and get back to you. |
@ArpitKotecha. So are the changes done by me still valid? |
api/models/Reading.js
Outdated
}, | ||
tankLocation: { | ||
required: true, | ||
type: String |
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.
Use GeoJSON
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.
@ArpitKotecha. I have made the necessary changes. Please review.
@ArpitKotecha @vinitshahdeo @amaaniqbal - A gentle follow up on this. |
Congrats on merging your first pull request! 🙌🎉⚡️ |
@ArpitKotecha, Please add the required labels for this PR, as my scores aren't getting reflected. |
Description
Fixes #162
Type of change
Please delete options that are not relevant.
Checklist:
Reviewer: Vinit Shahdeo and @amaaniqbal