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

On Sat, Jul 28, 2012 at 7:46 AM, Ana Cutillas <email address hidden> wrote:

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

Thanks for the explanation. I understand now.

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

Got it. This is OK. But document what the data structure looks like in
the comments (maybe it will be OK when you have the file that creates these
but someone else dealing with this needs to go somewhere to figure out what
these fields mean.

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

It could be easy to do that, but I'm actually concerned about the other
direction -- someone wants to use a text box that is optional. Now that's
not possible. It would be tricky to implement the creation UI for this, so
I think we should just let it go till later. I'd rather you work on things
we need rather than that.