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

Detect and select: mark\dismiss candidates #254

Merged
merged 3 commits into from Dec 1, 2017

Conversation

Projects
None yet
4 participants
@Serhiy-Shekhovtsov
Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 28, 2017

Implemented functionality for reviewing nodule candidates. Now user can set whether the nodule was marked for further analysis or not.

NOTE: this PR depends on #250 and has only 3 commits on top of it.

Reference to official issue

It's an improvement of #148

Screenshot:

detect-and-select-review
full size image

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov changed the title Detect and select: mark\dismiss implementation Detect and select: mark\dismiss candidates Nov 28, 2017

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:features/detect-and-select-mark branch from 65dfc29 to 8d8e27c Nov 28, 2017

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Nov 29, 2017

Works like a charm! Rebasing should sort out the errors :)

response = self.client.get(url)
response_dict = response.json()
self.assertEqual(response_dict["response"], "Candidate {} was dismissed".format(candidate.id))
self.assertEqual(response.status_code, status.HTTP_200_OK)

This comment has been minimized.

@reubano

reubano Nov 29, 2017

Contributor

Was there not a way to modify the tests for the new review_candidate and candidates_info routes?

This comment has been minimized.

@reubano

reubano Nov 29, 2017

Contributor

I have changed it to return the updated candidate.

So how about something like this...

def test_candidates_mark(self):
    candidate = CandidateFactory()
    url = reverse('review-candidate', kwargs={'candidate_id': candidate.id})
    response = self.client.post(url, json.dumps({'review_result': enums.CandidateReviewResult.MARKED}), 'application/json')
    response_dict = response.json()
    self.assertEquals(response_dict, 'SOMETHING')
    self.assertEqual(response.status_code, status.HTTP_200_OK)

This comment has been minimized.

@reubano

reubano Nov 29, 2017

Contributor

The thing is - I am not sure what is the test coverage policy in our project at this stage.

I'd say for changes, just try to make sure the previous tests still pass (making modifications to the test cases where necessary).

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 29, 2017

Contributor

Yes, something like that should do. Also we'll need to update the property on local candidate and use CandidateSerializer(candidate, context={'request': None}) to make 'SOMETHING' :)
Ok, will put them back and fix 👍

This comment has been minimized.

@reubano

reubano Nov 29, 2017

Contributor

Sounds like a plan :). Doesn't need to be anything complex. Just something that lets you verify from the response that everything worked as expected. Also, since it's POST (and no longer GET) am I correct in assuming the status code won't be 200 anymore?

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 29, 2017

Contributor

I think it's still 200 in case of success :) https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200


candidate.refresh_from_db()

self.assertEquals(candidate.review_result, enums.CandidateReviewResult.DISMISSED)

This comment has been minimized.

@reubano

reubano Nov 29, 2017

Contributor

Ohh, I see the tests were just moved over here :) Is this a better location for them?

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 29, 2017

Contributor

I just saw test_update_nodule_lung_orientation located here and used it as an example. But, ideally, we would test both aspects - the server response and result state in DB. The thing is - I am not sure what is the test coverage policy in our project at this stage.

This comment has been minimized.

@reubano

reubano Nov 29, 2017

Contributor

Yes, I do now see the tests are a bit different. What issues did you run into that you decided to remove the above ones?

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Nov 29, 2017

Contributor

The API has been changed. It used to return only a message about marking\dismissing, I have changed it to return the updated candidate. It's not like I was not able to fix the test because of some issue. Just found an example with test_update_nodule_lung_orientation and added the same type of test instead of old one.

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov force-pushed the Serhiy-Shekhovtsov:features/detect-and-select-mark branch from 8d8e27c to cc6ca74 Nov 29, 2017

@lamby lamby merged commit c4db12e into drivendataorg:master Dec 1, 2017

2 checks passed

concept-to-clinic/cla @Serhiy-Shekhovtsov has signed the CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Dec 1, 2017

I still have to add API test for this feature. Will make a new PR for that.

@api_view(['POST'])
def review_candidate(request, candidate_id):
try:
review_result = json.loads(request.body)['review_result']

This comment has been minimized.

@isms

isms Dec 3, 2017

Contributor

This comment has been minimized.

@Serhiy-Shekhovtsov

Serhiy-Shekhovtsov Dec 5, 2017

Contributor

@isms thanks for sharing. I can see you've updated the code to support partial_update.
Btw, the append_files_to_candidate is rather a workaround. We should load all files for selected case only once at case start.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 4, 2017

I still have to add API test for this feature. Will make a new PR for that.

@Serhiy-Shekhovtsov if you don't have time to do this fairly soon, can you create an issue for it?

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Dec 4, 2017

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Dec 4, 2017

@isms from my understanding, those are new DB state tests. The previous server response tests were removed. Are you saying these are actually one in the same?

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Dec 5, 2017

Guys, test are a little bit different. Old tests were checking the API output while new one is testing outcome(the DB result).

@Serhiy-Shekhovtsov Serhiy-Shekhovtsov referenced this pull request Dec 5, 2017

Merged

fixed candidate select\deselect API test #260

1 of 1 task complete

@reubano reubano referenced this pull request Dec 12, 2017

Closed

Add missing API server response tests #266

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment