-
Notifications
You must be signed in to change notification settings - Fork 4
Add checks for extra field in model create #766
Add checks for extra field in model create #766
Conversation
vms/shift/utils.py
Outdated
@@ -36,6 +36,9 @@ def create_event_with_details(event): | |||
Creates and returns event with passed name and dates | |||
""" | |||
e1 = Event(name=event[0], start_date=event[1], end_date=event[2]) |
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.
Instead of modifying attributes afterwards won't it be a better practice to pass it during Event init itself.
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.
This is the hacky way you suggested in meeting so that only 1 test is changed otherwise we'll have to modify like every test done till now. Also this will anyhow get changed when we implement dictionaries.
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.
Why are we not moving to dicts rightaway?
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.
Because moving to dictionaries has a lot of dependency and will need existing tests to be completed first so we can tackle all problem at once.
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.
You can have a check of len(event) by if else inside which you call the Event constructor as done in b4afaff
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.
Okay 👍
@Monal5031 As per your comment on my PR, I think you must include these fields in error message function. create locators etc. |
Yup I am, this is a stretch goal for this week. Thanks for reminding again :) |
5853a10
to
13d0db2
Compare
13d0db2
to
bbac88d
Compare
e3733e3
to
6f84f5a
Compare
@naman1901 @ayushgarg1804 @MehaKaushik @pavankm The checks for fields in shift app (model checks) will be added in #726 and for administrator in #780 . For admin and volunteer the checks had previously been done in #724 . |
4ad80f7
to
13b3af8
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.
Looks good!
13b3af8
to
f92bc33
Compare
ping @naman1901 @MehaKaushik @ayushgarg1804 Please review. |
@Monal5031 there's a test which is failing |
Same thing as in the documentation PR. @ayushgarg1804 will restart it in a bit. |
f92bc33
to
f5382d6
Compare
Description
Until now only mandatory fields were being saved but we should add checks for other fields too, at least in 1 test. Since these are non-mandatory fields and we'll be shifting to using dicts instead of lists I've used a workaround for now (using length compare) while creating the event. In future we'll need to modify it
Fixes #752
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
Checklist:
Delete irrelevant options.
Code/Quality Assurance Only