Code review comment for lp:~acoconut/systers/flexible_essays

Revision history for this message
Robin J (robin-jeffries) wrote :

for this line in listinfo.py
question_name = "'"+question[1]+"'"

I don't understand why you are enclosing this in 's? question[1] is a string and when you assign it to question_name it will still be a string. Am I missing something?

for this line in subscribe.py
essay.append((question[1], answer, question[5]))

I'm not sure why you need question[5] (which, unless you changed the input data structure doesn't exist. I think you mean question[4], which is the relative position, right? -- remember, it's zero based addressing). I don't think you need to know the actual position (except when you write the questions to the page in listinfo.py), you just need to be sure that each answer gets associated with the right question. You may need to put the version number in here, but I suggest you wait till you know how you are going to use that (for which you need to write the code that allows the admin to change the questions asked)

For the new code you put in subscribe.py, the way you have it written, the subscriber has to enter an answer to all the questions except the checkboxes. That's OK for our purposes, but it's not especially general. I think that for now, put in a comment to remind us all that this is true, and go on. We'll find out later if this is a problem.

I think that this is going to work -- you need to test whether they entered any data anyway, so you need to do most of this for that check.

What's going on here:
question_index = 0
if (isinstance(essay, str)): #There's only one question, so it comes as a string.
        form = DlistUtils.FormQuestion(mlist)
        form.recordAnswer(email, question_index, essay, 1)

This is completely different from what you do in the list case, I am confused. Did you forget to update this?

It looked to me like none of the stuff in DlistUtils.py had changed since the last round, so I didn't look at it seriously. Let me know if I missed something.

review: Needs Fixing

« Back to merge proposal