Issue 83043: Melange Surveys: Views, request processing and more.
Patch Set 1 #
Total comments: 32
Patch Set 2 : Fix most items, added some breathing space :) #
Total comments: 8
Patch Set 3 : Fixed style issues, use ACL for menu. #MessagesTotal messages: 5
As a general comment I suggest using a bit more whitespace to group code inside methods for readability. http://codereview.appspot.com/83043/diff/1/2 File app/soc/views/models/survey.py (right): http://codereview.appspot.com/83043/diff/1/2#newcode3 Line 3: # Copyright 2008 the Melange authors. As said in email, 2009 :). http://codereview.appspot.com/83043/diff/1/2#newcode29 Line 29: import StringIO Shouldn't this be grouped up with csv and datetime? http://codereview.appspot.com/83043/diff/1/2#newcode40 Line 40: from soc.logic.models.survey import getRoleSpecificFields, GRADES Every import should be on a seperate line. http://codereview.appspot.com/83043/diff/1/2#newcode52 Line 52: from soc.views.models import base All the soc imports should be grouped together. http://codereview.appspot.com/83043/diff/1/2#newcode88 Line 88: TODO: Read/Write Access Needs to Match Survey Is this still something that needs to be done? http://codereview.appspot.com/83043/diff/1/2#newcode150 Line 150: # so saving the whole form wouldn't be necessary. Has this been given some thought? If so could we condense the TODO and move it out of the dictionary definition. http://codereview.appspot.com/83043/diff/1/2#newcode207 Line 207: # this won't work -- there's *always* a survey entity. We want to Capitalize multiline comments. http://codereview.appspot.com/83043/diff/1/2#newcode236 Line 236: # No recorded answers, we're either past deadline or want to see answers "No" should not be capitalized. http://codereview.appspot.com/83043/diff/1/2#newcode251 Line 251: # midterm survey I still feel that code specifically for midterms doesn't belong here. Might need to discuss some options how to seperate normal surveys from the ones used to grade students. http://codereview.appspot.com/83043/diff/1/2#newcode255 Line 255: # Set help and status text Capitalization again. http://codereview.appspot.com/83043/diff/1/2#newcode275 Line 275: # Check deadline, see check for opening below And here. http://codereview.appspot.com/83043/diff/1/2#newcode281 Line 281: # Check if user can edit this survey And here. http://codereview.appspot.com/83043/diff/1/2#newcode288 Line 288: # Check if we're past the opening date And here. http://codereview.appspot.com/83043/diff/1/2#newcode360 Line 360: # Remove deleted properties from the model I think I have made my point :D. http://codereview.appspot.com/83043/diff/1/2#newcode523 Line 523: # don't understand what you're saying about permissions This dicussion needs to be cleaned out. http://codereview.appspot.com/83043/diff/1/2#newcode620 Line 620: #XXX Ditto for this redirect XXX ?
Sign in to reply to this message.
Most items fixed. I tried to add some breathing space, but was unsure at some places (always feeling like adding too many NLs). PTAL :) http://codereview.appspot.com/83043/diff/1/2 File app/soc/views/models/survey.py (right): http://codereview.appspot.com/83043/diff/1/2#newcode3 Line 3: # Copyright 2008 the Melange authors. On 2009/06/20 12:00:29, ljvderijk wrote: > As said in email, 2009 :). Done. http://codereview.appspot.com/83043/diff/1/2#newcode29 Line 29: import StringIO On 2009/06/20 12:00:29, ljvderijk wrote: > Shouldn't this be grouped up with csv and datetime? Done. http://codereview.appspot.com/83043/diff/1/2#newcode40 Line 40: from soc.logic.models.survey import getRoleSpecificFields, GRADES On 2009/06/20 12:00:29, ljvderijk wrote: > Every import should be on a seperate line. Done, but because we use surveys.getRoleSpecificFields now. http://codereview.appspot.com/83043/diff/1/2#newcode52 Line 52: from soc.views.models import base On 2009/06/20 12:00:29, ljvderijk wrote: > All the soc imports should be grouped together. Done. http://codereview.appspot.com/83043/diff/1/2#newcode88 Line 88: TODO: Read/Write Access Needs to Match Survey On 2009/06/20 12:00:29, ljvderijk wrote: > Is this still something that needs to be done? Not sure, but definitely not docstring material. Moved to comments. James, can we get rid of these? http://codereview.appspot.com/83043/diff/1/2#newcode150 Line 150: # so saving the whole form wouldn't be necessary. On 2009/06/20 12:00:29, ljvderijk wrote: > Has this been given some thought? If so could we condense the TODO and move it > out of the dictionary definition. This actually works now, so I've removed the TODO. I think we should stay with the hidden field for now: many uses, so they could all have a similar fix (like jquery data or the AJAXy saving). http://codereview.appspot.com/83043/diff/1/2#newcode207 Line 207: # this won't work -- there's *always* a survey entity. We want to On 2009/06/20 12:00:29, ljvderijk wrote: > Capitalize multiline comments. Done. http://codereview.appspot.com/83043/diff/1/2#newcode236 Line 236: # No recorded answers, we're either past deadline or want to see answers On 2009/06/20 12:00:29, ljvderijk wrote: > "No" should not be capitalized. Oops, I used the wrong code-style, almost all Capitalized single-line comments are my fault. Fixed. http://codereview.appspot.com/83043/diff/1/2#newcode251 Line 251: # midterm survey On 2009/06/20 12:00:29, ljvderijk wrote: > I still feel that code specifically for midterms doesn't belong here. Might need > to discuss some options how to seperate normal surveys from the ones used to > grade students. I think James meant 'gradable survey' here. I think the discussion is still relevant and one of our top priorities now, so I'll ping James on IRC and keep our ML discussion :) http://codereview.appspot.com/83043/diff/1/2#newcode255 Line 255: # Set help and status text On 2009/06/20 12:00:29, ljvderijk wrote: > Capitalization again. Done. http://codereview.appspot.com/83043/diff/1/2#newcode275 Line 275: # Check deadline, see check for opening below On 2009/06/20 12:00:29, ljvderijk wrote: > And here. Done. http://codereview.appspot.com/83043/diff/1/2#newcode281 Line 281: # Check if user can edit this survey On 2009/06/20 12:00:29, ljvderijk wrote: > And here. Done. http://codereview.appspot.com/83043/diff/1/2#newcode288 Line 288: # Check if we're past the opening date On 2009/06/20 12:00:29, ljvderijk wrote: > And here. Done. http://codereview.appspot.com/83043/diff/1/2#newcode360 Line 360: # Remove deleted properties from the model On 2009/06/20 12:00:29, ljvderijk wrote: > I think I have made my point :D. Done. http://codereview.appspot.com/83043/diff/1/2#newcode523 Line 523: # don't understand what you're saying about permissions On 2009/06/20 12:00:29, ljvderijk wrote: > This dicussion needs to be cleaned out. Done. I'll move them to templates once we base feature works. http://codereview.appspot.com/83043/diff/1/2#newcode620 Line 620: #XXX Ditto for this redirect On 2009/06/20 12:00:29, ljvderijk wrote: > XXX ? Wrong code-style for 'here be sucky code'. Fixed
Sign in to reply to this message.
Just nitpicking here. I wonder how much of this is stable because James is planning on seperating surveys from the ones that have grades. http://codereview.appspot.com/83043/diff/4/1003 File app/soc/views/models/survey.py (right): http://codereview.appspot.com/83043/diff/4/1003#newcode141 Line 141: # survey_html: save survey content when the POST fails, so fields remain in UI This line is too long. http://codereview.appspot.com/83043/diff/4/1003#newcode472 Line 472: # is needed No need for capitalization. http://codereview.appspot.com/83043/diff/4/1003#newcode661 Line 661: # TODO(ajaksu) One alternative would be to store the user key as an id attr This line is too long. http://codereview.appspot.com/83043/diff/4/1003#newcode742 Line 742: # Bail out early if survey_records.run() is empty Shouldn't be capitalized.
Sign in to reply to this message.
Thanks for the comments, Lennie! I've added a bit of new code to only show surveys the current user can read in the menu. We have a checkIsSurveyReadable method in views.helper.access.Checker, but I can't make it work for this :( http://codereview.appspot.com/83043/diff/4/1003 File app/soc/views/models/survey.py (right): http://codereview.appspot.com/83043/diff/4/1003#newcode141 Line 141: # survey_html: save survey content when the POST fails, so fields remain in UI On 2009/06/21 09:41:16, ljvderijk wrote: > This line is too long. Done. http://codereview.appspot.com/83043/diff/4/1003#newcode472 Line 472: # is needed On 2009/06/21 09:41:16, ljvderijk wrote: > No need for capitalization. Done. http://codereview.appspot.com/83043/diff/4/1003#newcode661 Line 661: # TODO(ajaksu) One alternative would be to store the user key as an id attr On 2009/06/21 09:41:16, ljvderijk wrote: > This line is too long. Done. http://codereview.appspot.com/83043/diff/4/1003#newcode742 Line 742: # Bail out early if survey_records.run() is empty On 2009/06/21 09:41:16, ljvderijk wrote: > Shouldn't be capitalized. Done.
Sign in to reply to this message.
On 2009/06/21 09:41:16, ljvderijk wrote: > I wonder how much of this is stable because James is planning on seperating > surveys from the ones that have grades. Forgot to reply to this :) IMO, if we make a few changes so this View knows how to dispatch ACLs based on a 'kind' property and handle links between surveys (neither major changes AFAICS), most of this is stable. If James decides to delegate either the survey links or support for different survey styles to me, I think I can do it and post for review before our Monday deadline.
Sign in to reply to this message.
|
