-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
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.
Need some changes, multiple instances are possible for following.
vms/pom/pages/eventsPage.py
Outdated
@@ -113,6 +112,9 @@ def go_to_create_organization_page(self): | |||
def get_deletion_box(self): | |||
return self.element_by_class_name(self.elements.DELETION_BOX) | |||
|
|||
def get_delete_element(self, relative): |
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.
rename to get_delete_event_element.
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.
Done.
vms/event/tests/test_event.py
Outdated
|
||
def check_error_messages(self): | ||
event_details_page = self.event_details_page | ||
error_message = 'This field is required.' |
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.
Make this a variable inside EventsPage
itself (multiple uses might be possible.)
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.
Done.
vms/event/tests/test_event.py
Outdated
event_details_page.go_to_events_page() | ||
|
||
self.assertEqual(event_details_page.get_message_context(), | ||
'There are currently no events. Please create events first.') |
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.
Same here, class var in EventsPage
.
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.
Done.
vms/event/tests/test_event.py
Outdated
event_details_page.fill_event_form(event_start_after_end) | ||
|
||
# Check error. | ||
self.assertEqual(event_details_page.get_event_start_date_error(), 'Start date must be before the end date') |
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.
Same here.
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.
Done.
vms/event/tests/test_model.py
Outdated
self.assertEqual(str(event_in_db.start_date), event[1]) | ||
self.assertEqual(str(event_in_db.end_date), event[2]) | ||
|
||
# Checks are missing from model class |
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.
Raise issue after confirmation in tomorrow's meeting.
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.
Workaround Solution found:
model validation checks aren't performed until a full_clean
is called explicitly from code/command-line, Forms perform full_clean
on submit thats why the error messages are shown. Thus model.save()
will save the instance unless there not-null constraint is broken.
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.
Does it make sense to use the workaround for now and file an issue to fix the bug later?
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.
Well its not a workaround per se, as I explained in the later part that model.save() doesnt actually executes the validation checks we need clean or full clean for it which is done by default in forms so there we can see an error while saving i.e.form.save but not in model.save. Ideally we should have a custom clean function in model to raise error while model save.
vms/event/tests/test_model.py
Outdated
self.assertEqual(event_in_db.name, 'new-event-name') | ||
self.assertEqual(len(Event.objects.all()), 1) | ||
|
||
# Clear function needs to be implemented for this. |
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.
Same here.
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.
Workaround Solution found:
model validation checks aren't performed until a full_clean
is called explicitly from code/command-line, Forms perform full_clean
on submit thats why the error messages are shown. Thus model.save()
will save the instance unless there not-null constraint is broken.
vms/event/tests/test_shiftSignUp.py
Outdated
@@ -172,77 +157,77 @@ def test_shift_sign_up_with_outdated_shifts(self): | |||
|
|||
# on jobs page | |||
sign_up_page.click_to_view_shifts() | |||
self.assertEqual(sign_up_page.get_info_box().text,'There are currently no shifts for the job job.') | |||
self.assertEqual(sign_up_page.get_info_box().text, 'There are currently no shifts for the job job.') |
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.
Class var in EventsPage
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.
Done.
vms/event/tests/test_shiftSignUp.py
Outdated
shift_1 = ["2016-05-11","9:00","15:00",6,Job.objects.get(name = 'job')] | ||
s1 = create_shift_with_details(shift_1) | ||
shift_1 = ["2016-05-11", "9:00", "15:00", 6, Job.objects.get(name='job')] | ||
create_shift_with_details(shift_1) |
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.
If something is returned, it is good practice to store the return value even if you're not using it. Someone else might want it later, but this way they'll think the function returns nothing.
vms/event/tests/test_shiftSignUp.py
Outdated
v2 = create_volunteer_with_details(volunteer_2) | ||
|
||
# Assign shift to the volunteer | ||
vol_shift = register_volunteer_for_shift_utility(s2, v2) | ||
register_volunteer_for_shift_utility(s2, v2) |
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.
Capture return value
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.
codacy was marking it as unused variable and failing. 😞
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.
I'll delete codacy comments, not to worry!
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.
Please update us on the workaround/bugfix issue.
It is good practice to capture return values even if you don't use them.
Review awaited from Anjali, Update and finalise PR and ping me on Slack @Monal5031 |
b3fbf6b
to
82ccfda
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.
LGTM
Description
Tested event app
Fixes #702
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
To verify that tests are non-breaking change in job app use:
and to overall project use:
Checklist:
Code/Quality Assurance Only