-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add Submission Review Serializer #1455
Add Submission Review Serializer #1455
Conversation
- Add get_note_text method - Add note_text property
- Use unicode_literals from __future__
- Add new test - test_submission_review_create
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'm not sure we are testing how this new serializer will behave in real life? In real life, the Note
object will not already be existing and will need to be created.
instance = Instance.objects.first() | ||
note = Note( | ||
instance=instance, | ||
instance_field="", |
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.
Would be cool to use this _review_status
as here #1428 (comment)
|
||
note.save() | ||
|
||
data = { |
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.
Is this likely to be the kind of data that we get when creating submission reviews?
- Add custom create method - Add custom update method - Add Tests
if note_text is None: | ||
raise exceptions.ValidationError({ | ||
'note': | ||
'Can\'t reject a submission without 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.
Can we please get this string from a common tags/strings file?
note_data = validated_data.pop('note') | ||
note_data['instance'] = validated_data.get('instance') | ||
note_data['created_by'] = validated_data.get('created_by') | ||
note_data['instance_field'] = "_review_status" |
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.
Lets get this string from a common tags or something file. Please look at how the logger.Instance fields are gotten. I think they come from onadata.libs.utils.common_tags
self.assertTrue(Note.objects.filter(instance=instance).exists()) | ||
|
||
note = submission_review.note | ||
self.assertEqual(instance, submission_review.instance) |
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.
it would be cool to have this more consistent. Maybe expected values on the left and received values on the right?
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.
Updated the tests to be more consistent in the latest commits.
- Add COMMENT_REQUIRED and SUBMISSION_REVIEW_INSTANCE_FIELD tag - Update Tests
8432d19
to
7e0a6d5
Compare
note_data['created_by'] = validated_data.get('created_by') | ||
note_data['instance_field'] = SUBMISSION_REVIEW_INSTANCE_FIELD | ||
|
||
note = NoteSerializer.create( |
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.
Should this be?
note_serializer = NoteSerializer(data=note_data)
note_serializer.save()
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.
We don't necessarily need the Serializer to validate the data as such i changed this to
Note.objects.create(**note_data)
with self.assertRaises(ValidationError) as no_comment: | ||
SubmissionReviewSerializer().validate(data) | ||
|
||
no_comment_error_detail = no_comment.exception.detail['note'] |
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.
The two lines should probably be under the with
block.
Custom update method for SubmissionReviewSerializer | ||
""" | ||
note = instance.note | ||
note_data = validated_data.pop('note') |
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.
Is the note not being updated? Like a change in the text?
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.
It is being updated. We get the note_data
in line 59 and change the notes text in line 61
No description provided.