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

#34 Select Lung Orientation #101

Merged
merged 1 commit into from Sep 23, 2017

Conversation

Projects
None yet
4 participants
@WGierke
Copy link
Contributor

WGierke commented Sep 9, 2017

This PR adds the ability to select whether a nodule belongs to the right or to the left lung.

Description

I added an accordion view of all nodules with a drop-down menu to select in which part of the lung a nodule is located.

Reference to official issue

This addresses #34 .

Motivation and Context

This should be a simple entry point for adding information to a nodule.

How Has This Been Tested?

I wrote a test that checks whether POSTing to the new back-end endpoint results in an appropriately updated nodule entity. Also, I tested it manually.

Screenshots:

screenshot from 2017-09-09 23-05-11

CLA

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

@WGierke WGierke force-pushed the WGierke:34_set_lung_orientation branch from 4af17b3 to 6daf30b Sep 9, 2017

@@ -54,3 +54,11 @@ class Meta:
read_only_fields = ('created',)

centroid = ImageLocationSerializer()

def create(self, validated_data):
case_data = validated_data.pop('case')

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

(Similar to previous PR review; why pop, etc. etc.)

@@ -55,3 +59,13 @@ def candidate_mark(request, candidate_id):

def candidate_dismiss(request, candidate_id):
return JsonResponse({'response': "Candidate {} was dismissed".format(candidate_id)})


@csrf_exempt

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

Why? (if justified, please add a comment the code so it's there forever!)


@csrf_exempt
def nodule_update(request, nodule_id):
lung_orientation = json.loads(request.body)['lung_orientation']

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

Feel like we want some validation here or at least a try […] except *g*

@@ -39,3 +51,5 @@ class Nodule(models.Model):
candidate = models.OneToOneField(Candidate, on_delete=models.CASCADE, null=True)

centroid = models.OneToOneField('images.ImageLocation', on_delete=models.CASCADE)

lung_orientation = models.CharField(max_length=1, choices=LungOrientations.choices(), default='0')

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

CharField smells wrong to me given that we can want to enforce numbers.

In addition the default should prbobably be LungOrientations.none rather than "0", if you see the semantic difference.

self.assertEquals(nodule.lung_orientation, '0')

# Set lung orientation to left
self.client.post("/api/nodules/{}/update".format(nodule.id), json.dumps({'lung_orientation': 'left'}),

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

I think we should use ``reverse``` even here.

from enum import Enum


class ChoiceEnum(Enum):

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

There are a number of Django enum fields - did you look into them at all? They seem to do this stuff a little nicer, or at least with less bus-factor!

{% endblock %}

This comment has been minimized.

@lamby

lamby Sep 10, 2017

Contributor

Whitespace ;)

try:
lung_orientation = json.loads(request.body)['lung_orientation']
except:
return JsonResponse({'response': "An error occurred."})

This comment has been minimized.

@lamby

lamby Sep 11, 2017

Contributor

This will return HTTP 200 now

},
update: function(nodule) {
this.$http.post(nodule.url + "update",
{ csrfmiddlewaretoken: $("input[name=csrfmiddlewaretoken]").val(),

This comment has been minimized.

@lamby

lamby Sep 14, 2017

Contributor

I wonder if we can do ths globally; kinda ugly to do it like this every time IMHO. jQuery has a global ajax handler- could you file a new issue?

This comment has been minimized.

@WGierke

WGierke Sep 14, 2017

Contributor

I filed issue #114 for that.

This comment has been minimized.

@isms

isms Sep 17, 2017

Contributor

Why would we use jQuery for this?

There's a simple way to set this up for Vue and Django:

Vue.http.headers.common['X-CSRFToken']

Where X-CSRFToken is a header coming from Django.

@WGierke WGierke referenced this pull request Sep 14, 2017

Closed

Always pass CSRF tokens when posting with jQuery #114

3 of 3 tasks complete
def nodule_update(request, nodule_id):
try:
lung_orientation = json.loads(request.body)['lung_orientation']
except:

This comment has been minimized.

@lamby

lamby Sep 15, 2017

Contributor

"Naked" try-except here? :)

@classmethod
def reversed_choices_dict(self):
"""Return the dictionary ORIENTATION->NUMBER"""
return dict([(y, x) for x, y in self.choices()])

This comment has been minimized.

@lamby

lamby Sep 15, 2017

Contributor

Just ooi, have you seen the nifty:

{y: x for x, y in self.choices()}

syntax? :)

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Sep 17, 2017

Thanks for the feedback @lamby! I already started to implement it. However, I'm a bit blocked by the fact that currently the Vue.js front-end neither has a router and other pages than the "Open Image" page, nor does jQuery seem to work (when trying to run $('select[name=lung_orientation]').val(), I'm getting the error '$' is not defined, when trying it with window.$... I'm getting a window.$ is not a function). @isms do you think you could have a look into that? I'm a complete novice when it comes to Vue :/ Otherwise I'd be supposed to wait for #110 to be completely implemented.

@antonow

This comment has been minimized.

Copy link
Contributor

antonow commented Sep 18, 2017

@WGierke and @isms, I would like to propose removing jQuery (and bootstrap.js) from the project altogether. Vue alone is able to handle all the front-end reactivity that bootstrap does in their JS-supported elements with less code. Also, jQuery's ajax isn't well suited to setting default data params -- see the convoluted solutions described here. Instead we could use axios, which makes it easy to set base configuration options including options for xsrfTokens and xsrfHeaders. Or we could use Vue's built in http functionality as @isms suggested above.

Anyway, I am close to done with a PR that should address #110, and I've integrated/ported the frontend code from this PR for which I'll be sure to credit @WGierke. I hope to have that ready for review soon, but I wanted to give my opinion regarding using jQuery after reading through the most recent comments on this PR.

@WGierke WGierke force-pushed the WGierke:34_set_lung_orientation branch 4 times, most recently from 4d57f9a to de8ae4c Sep 18, 2017

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Sep 18, 2017

@antonow This sounds great since I'm a giant novice when it comes to Vue :) I removed my front-end code again and refactored the back-end code. Travis is still making some troubles although all tests pass locally ...

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Sep 18, 2017

@antonow concur completely on removing jQuery.

Let's make the CSRF question even simpler: per the design document, security concerns are out of scope so XSS is not a concern at the moment.

I suggest we disable the CSRF middleware in Django. Thoughts?

@WGierke WGierke force-pushed the WGierke:34_set_lung_orientation branch from de8ae4c to bccd591 Sep 18, 2017

@antonow antonow referenced this pull request Sep 19, 2017

Merged

Port existing views and add VueRouter and Axios #125

1 of 1 task complete
@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Sep 19, 2017

I suggest we disable the CSRF middleware in Django. Thoughts?

Ew! Also very very hard to add it back later in my experience...

@isms

This comment has been minimized.

Copy link
Contributor

isms commented Sep 20, 2017

Ew! Also very very hard to add it back later in my experience...

Fair, but this is out of scope. We specifically put auth out of bounds here. Is it worth a large effort to implement token based auth through DRF?

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Sep 21, 2017

Is it worth a large effort to implement token based auth through DRF?

I'm.. not sure. Perhaps going the CSRF route is preferable overall, but I don't quite see what's so hard about keeping it? Just make sure we include a CSRF token on every page (or header) and let javascript always add it as a parameter? This is very very common in Django land.

@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Sep 21, 2017

Letting Django respond to every request with a CSRF token seems pretty easy by e.g. adding
'config.middleware.ForceCsrfCookieMiddleware', to MIDDLEWARE and adding

from django.utils.deprecation import MiddlewareMixin
from django.middleware.csrf import get_token

class ForceCsrfCookieMiddleware(MiddlewareMixin):
    """
    Forces the CSRF cookie to be set for each request
    """
    def process_request(self, request):
        get_token(request)

in interface/config/middleware.py.
Reference

Regarding #125 , letting Vue (with axios) respond with the correct CSRF token seems to be as easy as

import axios from 'axios';
axios.defaults.xsrfCookieName = 'csrftoken'
axios.defaults.xsrfHeaderName = 'X-CSRFToken'

Reference
However, is CSRF really something that should block this PR about handling the lung orientation in the back-end? Shouldn't the CSRF token management be added in another PR?

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Sep 22, 2017

@WGierke
Thanks for looking into this. I knew the Django parts (and the associate jQuery global blah) but didn't know about the Vue part - thanks!

is CSRF really something that should block this PR about handling the lung orientation in the back-end? Shouldn't the CSRF token management be added in another PR?

Haha, absolutely. Please open another issue at the very least and copy-paste what you wrote here into there? Of course, free free to come up with a PR too.. ;)

(AIUI it was blocking this...?)

@WGierke WGierke referenced this pull request Sep 22, 2017

Closed

Global CSRF Protection #134

0 of 3 tasks complete
@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Sep 22, 2017

@lamby I filed #134 . You're right, it blocked this PR when the front-end of the lung orientation selection was still part of this PR. Since that moved to #125 (and the issue has been filed), I'd say that this PR shouldn't be blocked anymore?

@lamby lamby merged commit 30b3818 into drivendataorg:master Sep 23, 2017

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

lamby commented Sep 23, 2017

Thanks! Rebased :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment