Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Check your answers page with notes for additions, changes and deletions #923
Check your answers page with notes for additions, changes and deletions #923
Changes from 16 commits
4cf9d4b
baf2882
ee24073
4bacf6f
91d523c
04314cf
e327b60
b53b887
3c81be0
74a0aa4
f633c6e
eb69746
b7c1a38
8fb7589
b841327
31c88d5
bd3ea69
6647099
93c4286
f45d357
6c5ad16
3afead9
264238b
b5fd806
e685bce
d2986d1
ffb38e1
f1f4d5f
fa36f46
75a3cb8
b92d2be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we want to delete the note we ideally want to put it back to the state it was before we added one in the first place. If that is the case I think you need to call this instead.
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 agree, this should have been the approach. updated
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.
Wherever we use Sinon we need to add a
restore()
at the tope level in anafterEach()
block.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.
First, a NIT, adding some whitespace. But also this test is incorrect.
It is currently passing because we never set the property
session.note
but do set the propertysession.data.note
. If you update the test to check for that it will fail becausesession.data.note
is defined.You need to do this to fix the test and properly check the note has been deleted.
However, this still won't pass unless the
DeleteNoteService
is updated to drop thenote
property instead of setting it to an empty 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.
makes sense, updated