-
Notifications
You must be signed in to change notification settings - Fork 438
Fix unit route so not overwrite unspecified values - Team B #1560
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
Conversation
improving editing units
|
I created a test you can extend to cover this new behavior: AndrewTr0612#4 |
…-endpoint Stub out a basic test for the /edit endpoint
|
@AndrewTr0612 & @danielbachhuber I wanted to know if this PR is ready for my review or if I should hold off given the comments made. Thanks. |
@huss As of my conversation with them on Friday, they're going to try to address my comments first. |
|
@huss The PR is ready for review, I just finished addressing the comments. |
huss
left a comment
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.
Thanks to @AndrewTr0612, @anthonyduenez, @Nespina24 & @princetonn for their first contribution to OED. Overall, this looks good. I made a few comments to consider. Please let me know if you have any thoughts, something is not clear or I can help.
|
@huss The PR is ready for review. I loved your feedback, I did all the changes you recommended! |
huss
left a comment
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.
Thanks for addressing my comments. Review and testing found it works as desired. Congratulations to @AndrewTr0612, @anthonyduenez, @Nespina24 & @princetonn on having your work merged into OED.
Description
(This PR updates the /edit route in units.js so that unit fields are only updated when those parameters are specifically provided. Previously, the route set all unit values even when some were not passed in the request body, which could result in unintended changes.
Contributors
Thien An Truong, @AndrewTr0612 email: [email protected]
Anthony Duenez Ramirez, @anthonyduenez email: [email protected]
Nathan Espina, @Nespina24, email: [email protected]
Princeton Nguyen, @princetonn, email: [email protected]
Fixes #1534
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
This change addresses only the behavior of the /edit route in units.js. No additional validation are included.