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

On Sat, Jul 28, 2012 at 6:57 AM, Robin J <email address hidden> wrote:

> Review: Needs Fixing
>
> 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?
>

I needed to enclose the field names with ', otherwise I couldn't retrieve
the contents with the cgi function.

If I don't enclose it, the source code looks something like

...name = woman ...

instead of

...name = 'woman'

and it just doesn't work.

>
> 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)
>

Yes, sorry! I added an extra field to store the version already and I'm
already passing the version number. Right now it's always version 0, but
it's already there.

>
> 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 assumed an unchecked checkbox could be a good answer but getting rid of
that is really easy. I'll add the comments.

>
> 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?
>

Ah, yes, sorry. Ugh. Sorry. Updating it right now.

>
> 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.
>

Nothing changed.

> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/117141
> You are the owner of lp:~acoconut/systers/flexible_essays.
>