Merge lp:~acoconut/systers/flexible_essays into lp:systers

[Ana Cutillas]

I cleaned up DlistUtils.py, removing the function get_answer since we don't need it.

[Robin J (robin-jeffries)]

I would move the comment "added to support subscribe essays" up to the if statement, as right now it looks like just that one line was added for the essays.

I would also suggest that you mark lines that are just for your debugging with a comment like "debugging" so that a) I can tell that's what they are, and b) you can find them to remove them later. I assume lines 53, 55-57 are debugging statements?

Why is replacements['<mm-essay-box>'] always ''? Shouldn't you just get rid of it?

In subscribe.py you should make sure that the essay variable that is passed in is the same type (a list?) whether there is just one question or many (I can't see where you construct that data structure -- where does that happen?)

 (in subscribe.py) It seems pretty dangerous to have the question label just be the order in which you process the questions. Python does not guarantee that in a loop that says

for foo in foo_list:

that you will process each foo in the same order every time. Also, what happens if I reorder these questions (you don't have that functionality yet, but you need it)? Do you want them to have different question numbers? I think you want the question name (like 'talk') here.

You also seem to have hard coded the version to 1. Why is that?

In DlistUtils.py, the FormQuestion class, why are you putting together that command statement, that seems just to put something in the info log, when your next statement does it more directly? That confused me for quite a while.

If form_question_id is autoupdating, then what are you doing in line 144?

In DlistUtils.py, where did you add remove_dlist() I don't see it.

review: Needs Fixing

[Robin J (robin-jeffries)]

Just read your comments. Sorry.

1. You are telling me you have no control of whether the essay comes in as a list or a string. OK (at least I don't have the knowledge to override it)

2. version ought to be stored with the question. You would update the version whenever someone edits the question rather than replaces it (I would think an edit would be one of several types
    -wording change
    - answer type

So I would start when someone creates a question as version 1, and then when it is edited, that gets incremented. So it's part of the definition of the question (like the question text), and you don't need to store it in the pck file (every question could be a different version level.).

I think the big reason you don't understand version yet is that you haven't figured out what you are going to do with it. If you were to sketch out what the UI is going to look like for the administrator, when they go to approve these folks, and then think about what you want it to look like if you have two people with a) different versions of the same question, b) different questions, that might help you figure out where you want to keep the version number.

[Ana Cutillas (acoconut)]

On Fri, Jul 27, 2012 at 6:47 AM, Robin J <email address hidden> wrote:

> Review: Needs Fixing
>
> I would move the comment "added to support subscribe essays" up to the if
> statement, as right now it looks like just that one line was added for the
> essays.
>

OK, done.

>
> I would also suggest that you mark lines that are just for your debugging
> with a comment like "debugging" so that a) I can tell that's what they are,
> and b) you can find them to remove them later. I assume lines 53, 55-57
> are debugging statements?
>

Line 53 is not mine and it looks to me that when somebody is logging
through syslog for debugging, they write to the info file, in this line
they're writing to the error log, so maybe they wanted to keep a log of
this error every time it happens? I can add the if DEBUG_MODE statement if
you think it shouldn't be always stored. Let me know.

55-57 was junk, I got rid of it.

> Why is replacements['<mm-essay-box>'] always ''? Shouldn't you just get
> rid of it?
>

Yes, we're not using the replacements['mm-essay-box'] anymore, this might
mean some cleaning up is due in all the template related files. I'll leave
this for the end when I do the cleaning up.

> In subscribe.py you should make sure that the essay variable that is
> passed in is the same type (a list?) whether there is just one question or
> many (I can't see where you construct that data structure -- where does
> that happen?)
>
> (in subscribe.py) It seems pretty dangerous to have the question label
> just be the order in which you process the questions. Python does not
> guarantee that in a loop that says
>
> for foo in foo_list:
>
> that you will process each foo in the same order every time. Also, what
> happens if I reorder these questions (you don't have that functionality
> yet, but you need it)? Do you want them to have different question
> numbers? I think you want the question name (like 'talk') here.
>

Mmm... are you suggesting using the question name as the question ID? That
would work.

>
> You also seem to have hard coded the version to 1. Why is that?
>
> In DlistUtils.py, the FormQuestion class, why are you putting together
> that command statement, that seems just to put something in the info log,
> when your next statement does it more directly? That confused me for quite
> a while.
>

I know, it confused me too, but it's what every other class does! So I
thought it was something you guys had agreed on?

> If form_question_id is autoupdating, then what are you doing in line 144?
>

Line 144 is not mine or close to mine, I wrote around the 300 lines.

>
>
> In DlistUtils.py, where did you add remove_dlist() I don't see it.
>

I haven't done anything about removing dlists. Had we talk about something
like that?

From your other email:

2. version ought to be stored with the question. You would update the
> version whenever someone edits the question rather than replaces it (I
> would think an edit would be one of several types
> -wording change
> - answer type
>
> So I would start when someone creates a question as version 1, and then
> when it is edited, that gets incremented. So it's part of the definition
> of the qu...

[Ana Cutillas (acoconut)]

>
>
> In subscribe.py you should make sure that the essay variable that is
> passed in is the same type (a list?) whether there is just one question or
> many (I can't see where you construct that data structure -- where does
> that happen?)
> (in subscribe.py) It seems pretty dangerous to have the question label
> just be the order in which you process the questions. Python does not
> guarantee that in a loop that says
> for foo in foo_list:
> that you will process each foo in the same order every time. Also, what
> happens if I reorder these questions (you don't have that functionality
> yet, but you need it)? Do you want them to have different question
> numbers? I think you want the question name (like 'talk') here.
>
>
Mmm... are you suggesting using the question name as the question ID? That
> would work.

Ok, I remember now what the problem was with this.

There are two ways I could go about naming the fields in the form.

1. I could give them the questions names, i.e. woman, talk, etc. That would
go perfect when writing to the database because I could use that word as
the ID. BUT, if I do this, when I have to retrieve the fields I don't know
how many of them there are, or how to retrieve them all. I don't know if
there's anyway of changing the structure that Cgi uses to send the content
of the form, or how to add info to this structure.

2. I could give them all the same name: essay. This is what I'm doing now.
But then, you said that the for loop doesn't guarantee that I'll be going
through the questions always in the same order, and I can't store the
question name because I don't have it - cgi doesn't send it, it only sends
the content of the fields. Well, it sends the name of the checkboxes (It
doesn't send true or false, it either sends woman, or it doesn't send
anything)

So clearly the cgi approach is quite flakey for what we're trying to do
now... what are your thoughts?

>
> On Fri, Jul 27, 2012 at 6:47 AM, Robin J <email address hidden> wrote:
>
>> Review: Needs Fixing
>>
>> I would move the comment "added to support subscribe essays" up to the if
>> statement, as right now it looks like just that one line was added for the
>> essays.
>>
>
> OK, done.
>
>>
>> I would also suggest that you mark lines that are just for your debugging
>> with a comment like "debugging" so that a) I can tell that's what they are,
>> and b) you can find them to remove them later. I assume lines 53, 55-57
>> are debugging statements?
>>
>
> Line 53 is not mine and it looks to me that when somebody is logging
> through syslog for debugging, they write to the info file, in this line
> they're writing to the error log, so maybe they wanted to keep a log of
> this error every time it happens? I can add the if DEBUG_MODE statement if
> you think it shouldn't be always stored. Let me know.
>
> 55-57 was junk, I got rid of it.
>
>
>> Why is replacements['<mm-essay-box>'] always ''? Shouldn't you just get
>> rid of it?
>>
>
> Yes, we're not using the replacements['mm-essay-box'] anymore, this might
> mean some cleaning up is due in all the template related files. I'll leave
> this for the end when I do the cleaning up.
>
>
>> In subscribe.py you s...

[Ana Cutillas (acoconut)]

On Fri, Jul 27, 2012 at 8:12 PM, Ana Cutillas <email address hidden> wrote:

>
>> In subscribe.py you should make sure that the essay variable that is
>> passed in is the same type (a list?) whether there is just one question or
>> many (I can't see where you construct that data structure -- where does
>> that happen?)
>> (in subscribe.py) It seems pretty dangerous to have the question label
>> just be the order in which you process the questions. Python does not
>> guarantee that in a loop that says
>> for foo in foo_list:
>> that you will process each foo in the same order every time. Also, what
>> happens if I reorder these questions (you don't have that functionality
>> yet, but you need it)? Do you want them to have different question
>> numbers? I think you want the question name (like 'talk') here.
>>
>>
> Mmm... are you suggesting using the question name as the question ID? That
>> would work.
>
>
> Ok, I remember now what the problem was with this.
>
> There are two ways I could go about naming the fields in the form.
>
> 1. I could give them the questions names, i.e. woman, talk, etc. That
> would go perfect when writing to the database because I could use that word
> as the ID. BUT, if I do this, when I have to retrieve the fields I don't
> know how many of them there are, or how to retrieve them all. I don't know
> if there's anyway of changing the structure that Cgi uses to send the
> content of the form, or how to add info to this structure.
>

I could read the pck file again in the subscribe file, get the name and
number of questions and check against the info brought by the cgi module.
How does that sound?

> 2. I could give them all the same name: essay. This is what I'm doing now.
> But then, you said that the for loop doesn't guarantee that I'll be going
> through the questions always in the same order, and I can't store the
> question name because I don't have it - cgi doesn't send it, it only sends
> the content of the fields. Well, it sends the name of the checkboxes (It
> doesn't send true or false, it either sends woman, or it doesn't send
> anything)
>
> So clearly the cgi approach is quite flakey for what we're trying to do
> now... what are your thoughts?
>
>
>
>
>>
>> On Fri, Jul 27, 2012 at 6:47 AM, Robin J <email address hidden> wrote:
>>
>>> Review: Needs Fixing
>>>
>>> I would move the comment "added to support subscribe essays" up to the
>>> if statement, as right now it looks like just that one line was added for
>>> the essays.
>>>
>>
>> OK, done.
>>
>>>
>>> I would also suggest that you mark lines that are just for your
>>> debugging with a comment like "debugging" so that a) I can tell that's what
>>> they are, and b) you can find them to remove them later. I assume lines
>>> 53, 55-57 are debugging statements?
>>>
>>
>> Line 53 is not mine and it looks to me that when somebody is logging
>> through syslog for debugging, they write to the info file, in this line
>> they're writing to the error log, so maybe they wanted to keep a log of
>> this error every time it happens? I can add the if DEBUG_MODE statement if
>> you think it shouldn't be always stored. Let me know.
>>
>> 55-57 was junk, I got rid of...

[Ana Cutillas (acoconut)]

Oops, ignore this:

+ box_name = "essay.q"+str(index)
+ syslog('info', "Box name: %s", box_name)

I just deleted it.

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

> Ana Cutillas has proposed merging lp:~acoconut/systers/flexible_essays
> into lp:systers.
>
> Requested reviews:
> Robin J (robin-jeffries)
> beachbrake (wordsofagirl)
>
> For more details, see:
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/117141
>
> I made the changes I talked about in my emails earlier.
>
> Basically, I'm reading the pck file in subscribe.py and then I check every
> question name against the questions coming from the form (which are no
> longer all called essay, they each have its own name) and then I store the
> questions in the database with the question name as an ID for every
> question. The question number is also stored in the pickle file, read and
> added to the database.
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/117141
> You are the owner of lp:~acoconut/systers/flexible_essays.
>
> === modified file 'Mailman/Cgi/listinfo.py'
> --- Mailman/Cgi/listinfo.py 2009-08-14 15:41:17 +0000
> +++ Mailman/Cgi/listinfo.py 2012-07-27 22:13:21 +0000
> @@ -22,6 +22,7 @@
> import os
> import cgi
>
> +from Mailman import DlistUtils
> from Mailman import mm_cfg
> from Mailman import Utils
> from Mailman import MailList
> @@ -36,7 +37,6 @@
> i18n.set_language(mm_cfg.DEFAULT_SERVER_LANGUAGE)
>
>
> -
> def main():
> parts = Utils.GetPathPieces()
> if not parts:
> @@ -62,7 +62,6 @@
> list_listinfo(mlist, language)
>
>
> -
> def listinfo_overview(msg=''):
> # Present the general listinfo overview
> hostname = Utils.get_domain()
> @@ -152,7 +151,6 @@
> print doc.Format()
>
>
> -
> def list_listinfo(mlist, lang):
> # Generate list specific listinfo
> doc = HeadlessDocument()
> @@ -189,10 +187,29 @@
>
> # If activated, the content and presence of questions and a field to
> answer them. Added by Systers.
> try:
> - if mlist.essay_enabled:
> - replacements['<mm-questions>'] = """<TR>
> - <TD COLSPAN="3">""" + mlist.questions
> - replacements['<mm-essay-box>'] = TextArea('essay', '',
> rows=8, cols=60).Format() + """ </TD></TR>"""# Added to support
> subscribe essays
> + if mlist.essay_enabled: # Added to support subscribe essays
> + html_questions = []
> + index = 0
> + for question in mlist.questions:
> + box_name = "essay.q"+str(index)
> + syslog('info', "Box name: %s", box_name)
> + index = index + 1
> + question_name = "'"+question[1]+"'"
> + html_questions.append("""<TR>
> + <TD COLSPAN="3">""" + question[0] + """</TD>""")
> + if (question[2] == 'smallbox'):
> + html_questions.append("""<TD>""" +
> TextArea(question_name,\
> + '', rows=1, cols=60).Format() +
> """</TD></TR>""")
> + elif (question[2] == 'largebox'):
> + htm...

[Robin J (robin-jeffries)]

On Fri, Jul 27, 2012 at 10:43 AM, Ana Cutillas <email address hidden> wrote:

> On Fri, Jul 27, 2012 at 6:47 AM, Robin J <email address hidden> wrote:
>
> > Review: Needs Fixing
> >
> > I would move the comment "added to support subscribe essays" up to the if
> > statement, as right now it looks like just that one line was added for
> the
> > essays.
> >
>
> OK, done.
>
> >
> > I would also suggest that you mark lines that are just for your debugging
> > with a comment like "debugging" so that a) I can tell that's what they
> are,
> > and b) you can find them to remove them later. I assume lines 53, 55-57
> > are debugging statements?
> >
>
> Line 53 is not mine and it looks to me that when somebody is logging
> through syslog for debugging, they write to the info file, in this line
> they're writing to the error log, so maybe they wanted to keep a log of
> this error every time it happens? I can add the if DEBUG_MODE statement if
> you think it shouldn't be always stored. Let me know.
>
> 55-57 was junk, I got rid of it.
>

Apparently we are not talking about the same line numbers. I mean the ones
in the diff file.
index = 0

   and

box_name = "essay.q"+str(index)
syslog('info', "Box name: %s", box_name)
index = index + 1

but it looks like this is gone now.

>
>
> > Why is replacements['<mm-essay-box>'] always ''? Shouldn't you just get
> > rid of it?
> >
>
> Yes, we're not using the replacements['mm-essay-box'] anymore, this might
> mean some cleaning up is due in all the template related files. I'll leave
> this for the end when I do the cleaning up.
>

OK, I would suggest you put in a comment with at TODO to remind yourself.

>
>
> > In subscribe.py you should make sure that the essay variable that is
> > passed in is the same type (a list?) whether there is just one question
> or
> > many (I can't see where you construct that data structure -- where does
> > that happen?)
> >
> > (in subscribe.py) It seems pretty dangerous to have the question label
> > just be the order in which you process the questions. Python does not
> > guarantee that in a loop that says
> >
> > for foo in foo_list:
> >
> > that you will process each foo in the same order every time. Also, what
> > happens if I reorder these questions (you don't have that functionality
> > yet, but you need it)? Do you want them to have different question
> > numbers? I think you want the question name (like 'talk') here.
> >
>
> Mmm... are you suggesting using the question name as the question ID? That
> would work.
>

Yep, as you understood.

>
>
> >
> > You also seem to have hard coded the version to 1. Why is that?
>
>
> > In DlistUtils.py, the FormQuestion class, why are you putting together
> > that command statement, that seems just to put something in the info log,
> > when your next statement does it more directly? That confused me for
> quite
> > a while.
> >
>
> I know, it confused me too, but it's what every other class does! So I
> thought it was something you guys had agreed on?
>

Nope, I think that when Malveeka did the STORM code, this was just how she
set up her debug statements. We weren't doing code reviews that summer...

Feel free to ask when things...

[Robin J (robin-jeffries)]

On Fri, Jul 27, 2012 at 11:16 AM, Ana Cutillas <email address hidden> wrote:

> >
> >
> > In subscribe.py you should make sure that the essay variable that is
> > passed in is the same type (a list?) whether there is just one question
> or
> > many (I can't see where you construct that data structure -- where does
> > that happen?)
> > (in subscribe.py) It seems pretty dangerous to have the question label
> > just be the order in which you process the questions. Python does not
> > guarantee that in a loop that says
> > for foo in foo_list:
> > that you will process each foo in the same order every time. Also, what
> > happens if I reorder these questions (you don't have that functionality
> > yet, but you need it)? Do you want them to have different question
> > numbers? I think you want the question name (like 'talk') here.
> >
> >
> Mmm... are you suggesting using the question name as the question ID? That
> > would work.
>
>
> Ok, I remember now what the problem was with this.
>
> There are two ways I could go about naming the fields in the form.
>
> 1. I could give them the questions names, i.e. woman, talk, etc. That would
> go perfect when writing to the database because I could use that word as
> the ID. BUT, if I do this, when I have to retrieve the fields I don't know
> how many of them there are, or how to retrieve them all. I don't know if
> there's anyway of changing the structure that Cgi uses to send the content
> of the form, or how to add info to this structure.
>

If I'm understanding this right, when you retrieve the fields you want to
do something like

SELECT * from essay_table WHERE email_id = 'xxx' (or whatever key you have
for that person)

Then you have the full set. Again, you need to think about what you are
going to show to the admin, but I imagine a table whose rows are people who
have filled out subscription, and the columns are email, name, woman?,
agree to rules?, how woman in computing, (and then there are some radio
buttons about what to do with each row). You have both the names for the
columns and the values for each cell in that set of things you retrieved.

Go back to the list that I made for you on abiwt.org and subscribe some
more people, so you can see what things look like when an admin has to
approve them. You just need to be able to line up the content into the
right columns (which gets a bit tricky when a column is added, but other
than that, it should be OK -- and I think that you need to do separate
tables for people who have different questions, which might be where the
versioning comes in -- it may not have to do with an individual question
but have to do with the set of questions changing)

>
> 2. I could give them all the same name: essay. This is what I'm doing now.
> But then, you said that the for loop doesn't guarantee that I'll be going
> through the questions always in the same order, and I can't store the
> question name because I don't have it - cgi doesn't send it, it only sends
> the content of the fields. Well, it sends the name of the checkboxes (It
> doesn't send true or false, it either sends woman, or it doesn't send
> anything)
>

So definitely you can't do it that way,...

[Robin J (robin-jeffries)]

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

[Ana Cutillas (acoconut)]

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

> On Fri, Jul 27, 2012 at 10:43 AM, Ana Cutillas <email address hidden>
> wrote:
>
> > On Fri, Jul 27, 2012 at 6:47 AM, Robin J <email address hidden> wrote:
> >
> > > Review: Needs Fixing
> > >
> > > I would move the comment "added to support subscribe essays" up to the
> if
> > > statement, as right now it looks like just that one line was added for
> > the
> > > essays.
> > >
> >
> > OK, done.
> >
> > >
> > > I would also suggest that you mark lines that are just for your
> debugging
> > > with a comment like "debugging" so that a) I can tell that's what they
> > are,
> > > and b) you can find them to remove them later. I assume lines 53,
> 55-57
> > > are debugging statements?
> > >
> >
> > Line 53 is not mine and it looks to me that when somebody is logging
> > through syslog for debugging, they write to the info file, in this line
> > they're writing to the error log, so maybe they wanted to keep a log of
> > this error every time it happens? I can add the if DEBUG_MODE statement
> if
> > you think it shouldn't be always stored. Let me know.
> >
> > 55-57 was junk, I got rid of it.
> >
>
> Apparently we are not talking about the same line numbers. I mean the ones
> in the diff file.
> index = 0
>
> and
>
> box_name = "essay.q"+str(index)
> syslog('info', "Box name: %s", box_name)
> index = index + 1
>
> but it looks like this is gone now.
>
> Yes, I cleaned that up.

> >
> >
> > > Why is replacements['<mm-essay-box>'] always ''? Shouldn't you just
> get
> > > rid of it?
> > >
> >
> > Yes, we're not using the replacements['mm-essay-box'] anymore, this might
> > mean some cleaning up is due in all the template related files. I'll
> leave
> > this for the end when I do the cleaning up.
> >
>
> OK, I would suggest you put in a comment with at TODO to remind yourself.
>
>
> >
> >
> > > In subscribe.py you should make sure that the essay variable that is
> > > passed in is the same type (a list?) whether there is just one question
> > or
> > > many (I can't see where you construct that data structure -- where does
> > > that happen?)
> > >
> > > (in subscribe.py) It seems pretty dangerous to have the question label
> > > just be the order in which you process the questions. Python does not
> > > guarantee that in a loop that says
> > >
> > > for foo in foo_list:
> > >
> > > that you will process each foo in the same order every time. Also,
> what
> > > happens if I reorder these questions (you don't have that functionality
> > > yet, but you need it)? Do you want them to have different question
> > > numbers? I think you want the question name (like 'talk') here.
> > >
> >
> > Mmm... are you suggesting using the question name as the question ID?
> That
> > would work.
> >
>
> Yep, as you understood.
>
> >
> >
> > >
> > > You also seem to have hard coded the version to 1. Why is that?
> >
> >
> > > In DlistUtils.py, the FormQuestion class, why are you putting together
> > > that command statement, that seems just to put something in the info
> log,
> > > when your next statement does it more directly? That confused me for
> > quite
> > > a while.
> > >
> >
> > I k...

[Ana Cutillas (acoconut)]

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

> On Fri, Jul 27, 2012 at 11:16 AM, Ana Cutillas <email address hidden>
> wrote:
>
> > >
> > >
> > > In subscribe.py you should make sure that the essay variable that is
> > > passed in is the same type (a list?) whether there is just one question
> > or
> > > many (I can't see where you construct that data structure -- where does
> > > that happen?)
> > > (in subscribe.py) It seems pretty dangerous to have the question label
> > > just be the order in which you process the questions. Python does not
> > > guarantee that in a loop that says
> > > for foo in foo_list:
> > > that you will process each foo in the same order every time. Also,
> what
> > > happens if I reorder these questions (you don't have that functionality
> > > yet, but you need it)? Do you want them to have different question
> > > numbers? I think you want the question name (like 'talk') here.
> > >
> > >
> > Mmm... are you suggesting using the question name as the question ID?
> That
> > > would work.
> >
> >
> > Ok, I remember now what the problem was with this.
> >
> > There are two ways I could go about naming the fields in the form.
> >
> > 1. I could give them the questions names, i.e. woman, talk, etc. That
> would
> > go perfect when writing to the database because I could use that word as
> > the ID. BUT, if I do this, when I have to retrieve the fields I don't
> know
> > how many of them there are, or how to retrieve them all. I don't know if
> > there's anyway of changing the structure that Cgi uses to send the
> content
> > of the form, or how to add info to this structure.
> >
>
> If I'm understanding this right, when you retrieve the fields you want to
> do something like
>
> SELECT * from essay_table WHERE email_id = 'xxx' (or whatever key you have
> for that person)
>
> Then you have the full set. Again, you need to think about what you are
> going to show to the admin, but I imagine a table whose rows are people who
> have filled out subscription, and the columns are email, name, woman?,
> agree to rules?, how woman in computing, (and then there are some radio
> buttons about what to do with each row). You have both the names for the
> columns and the values for each cell in that set of things you retrieved.
>

So we basically want something like what mailman has in
/mailman/admin/listname/members but for pending users? Or do we want to add
columns to that table?

>
> Go back to the list that I made for you on abiwt.org and subscribe some
> more people, so you can see what things look like when an admin has to
> approve them. You just need to be able to line up the content into the
> right columns (which gets a bit tricky when a column is added, but other
> than that, it should be OK -- and I think that you need to do separate
> tables for people who have different questions, which might be where the
> versioning comes in -- it may not have to do with an individual question
> but have to do with the set of questions changing)
>
> I had no idea you had made a list for me. I'm testing everything on my
local machine. Should I use abiwt? And if so, how do I access it?

> >
> > 2. I co...

[Ana Cutillas (acoconut)]

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

[Robin J (robin-jeffries)]

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

>
> >
> > formQuestionID =
> > self.store.find(FormQuestion).max(FormQuestion.form_question_id)
> >
> >
> I am saving in formQuestionID the return value from storing which is the
> number that has been autoupdated. Does that make sense?
>

Yes, I think my problem was that I wasn't looking at the left side because
of the line break. My bad.

>
> >
> > I was refering to the line in rmlist
> > DlistUtils.remove_dlist(listname)
> > (which, taken with the lines above it, seems to be that you encapsulated
> > those lines into a method, but I didn't see it in the changes to
> > DlistUtils.py
> >
> >
> > I haven't touched rmlist at all. So I don't know how that changed :-/
>

OK, looks like someone else did this, but it's not in the repo. Next year
we will start everyone with a clean slate and eliminate these issues.

[Robin J (robin-jeffries)]

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.

[Robin J (robin-jeffries)]

getting down to small issues

results.append(_('To subscribe to this list, you must answer the questions asked directly above the large text box.'))

Need a different message, since it isn't always a large text box. How about 'To subscribe to this list, you must answer all the questions asked on this page.'

Clean up the command stuff (and the debugging message that emits that) in DlistUtils.py

review: Needs Fixing

[Robin J (robin-jeffries)]

in DlistUtils.py
 if DEBUG_MODE:
    syslog('info', 'DlistUtils(get_answer):Executing query:\n%s', command)

but command is no longer defined, right? I think you removed the wrong syslog statement.

Do you have a place where you use get_answer() yet? I'm trying to figure out how you are going to know the form_question_id

put
 syslog('info', "Result %s", result)
under an if DEBUG_MODE: guard -- you don't want to be filling up the info log with all these statements every time someone signs up.

Sorry to have missed some of these earlier -- it's hard to see everything going on in a review.

[Ana Cutillas (acoconut)]

I was writing get_answer because I thought I was going to need it to
populate the pending users table but I don't. So that whole function is
going away right now.

On Wed, Aug 1, 2012 at 5:13 AM, Robin J <email address hidden> wrote:

> in DlistUtils.py
> if DEBUG_MODE:
> syslog('info', 'DlistUtils(get_answer):Executing query:\n%s', command)
>
> but command is no longer defined, right? I think you removed the wrong
> syslog statement.
>
> Do you have a place where you use get_answer() yet? I'm trying to figure
> out how you are going to know the form_question_id
>
> put
> syslog('info', "Result %s", result)
> under an if DEBUG_MODE: guard -- you don't want to be filling up the info
> log with all these statements every time someone signs up.
>
> Sorry to have missed some of these earlier -- it's hard to see everything
> going on in a review.
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/117508
> You are the owner of lp:~acoconut/systers/flexible_essays.
>

[Robin J (robin-jeffries)]

Looks good. Now lets see how you use these....

review: Approve

[lp:~acoconut/systers/flexible_essays]

94. By Ana Cutillas <email address hidden> on 2012-08-04

Added changes to show the new essays in the pending subscriptions table.

Handle the possibility of a user trying to subscribe twice before the
subscription is reviewed.

95. By Ana Cutillas <email address hidden> on 2012-08-04

Added the functionality to check that the answers aren't longer than
what the database can hold.

96. By Ana Cutillas <email address hidden> on 2012-08-04

Added the error message saying that the user hasn't been subscribe to
the list when a user tries to subscribe with a pending subscription.

97. By Ana Cutillas <email address hidden> on 2012-08-07

Made the changes suggested by Robin: concerning error messages and
asserting the size of the answers.

Added the code to mark answers as accepted in the database when a user
is accepted.

98. By Ana Cutillas <email address hidden> on 2012-08-09

Changed the logic in the checking of answers in subscribe.py
Cleaned up DlistUtils.py

99. By Ana Cutillas <email address hidden> on 2012-08-09

Added support for setting answers to rejected in the database.

Unmerged revisions

99. By Ana Cutillas <email address hidden> on 2012-08-09

Added support for setting answers to rejected in the database.

98. By Ana Cutillas <email address hidden> on 2012-08-09

Changed the logic in the checking of answers in subscribe.py
Cleaned up DlistUtils.py

97. By Ana Cutillas <email address hidden> on 2012-08-07

Made the changes suggested by Robin: concerning error messages and
asserting the size of the answers.

Added the code to mark answers as accepted in the database when a user
is accepted.

96. By Ana Cutillas <email address hidden> on 2012-08-04

Added the error message saying that the user hasn't been subscribe to
the list when a user tries to subscribe with a pending subscription.

95. By Ana Cutillas <email address hidden> on 2012-08-04

Added the functionality to check that the answers aren't longer than
what the database can hold.

94. By Ana Cutillas <email address hidden> on 2012-08-04

Added changes to show the new essays in the pending subscriptions table.

Handle the possibility of a user trying to subscribe twice before the
subscription is reviewed.

93. By Ana Cutillas <email address hidden> on 2012-08-01

Removed the unused function get_answer from DlistUtils.py

92. By Ana Cutillas <email address hidden> on 2012-07-31

Cleaned up DlistUtils and changed the error message in subscribe. Added
comments.

91. By Ana Cutillas <email address hidden> on 2012-07-28

Cleaned up.

90. By Ana Cutillas <email address hidden> on 2012-07-27

Cleaned up listinfo.py

=== modified file 'Mailman/Cgi/admindb.py'
--- Mailman/Cgi/admindb.py	2009-07-31 10:49:01 +0000
+++ Mailman/Cgi/admindb.py	2012-08-04 00:41:20 +0000
@@ -285,11 +285,13 @@
     form.AddItem(Center(Header(2, _('Subscription Requests'))))
     table = Table(border=2)
     # The Essay is there to support subscription with essay, along with below references to "essay"
-    table.AddRow([Center(Bold(_('Address/name'))),
+    table_headers = ([Center(Bold(_('Address/name'))),
                   Center(Bold(_('Your decision'))),
-                  Center(Bold(_('Reason for refusal'))),
-                  Center(Bold(_('Essay')))
+                  Center(Bold(_('Reason for refusal')))
                   ])
+    for question in mlist.questions:
+        table_headers.append(Center(Bold(question[0])))
+    table.AddRow(table_headers)
     # Alphabetical order by email address
     byaddrs = {}
     for id in pendingsubs:
@@ -321,11 +323,12 @@
         # While the address may be a unicode, it must be ascii
         paddr = addr.encode('us-ascii', 'replace')
         # Added  by Systers to support essay feature. Lets the list admin see the essay the subscriber has entered.
-        table.AddRow(['%s<br><em>%s</em>' % (paddr, Utils.websafe(fullname)),
+        content_row = (['%s<br><em>%s</em>' % (paddr, Utils.websafe(fullname)),
                       radio,
-                      TextBox('comment-%d' % id, size=40),
-			essay
-                      ])
+                      TextBox('comment-%d' % id, size=40)])
+        for answer in essay:
+            content_row.append(answer[1])
+        table.AddRow(content_row)
         num += 1
     if num > 0:
         form.AddItem(table)

=== modified file 'Mailman/Cgi/listinfo.py'
--- Mailman/Cgi/listinfo.py	2009-08-14 15:41:17 +0000
+++ Mailman/Cgi/listinfo.py	2012-08-04 00:41:20 +0000
@@ -22,6 +22,7 @@
 import os
 import cgi
 
+from Mailman import DlistUtils
 from Mailman import mm_cfg
 from Mailman import Utils
 from Mailman import MailList
@@ -36,7 +37,6 @@
 i18n.set_language(mm_cfg.DEFAULT_SERVER_LANGUAGE)
 
 
-

 def main():
     parts = Utils.GetPathPieces()
     if not parts:
@@ -62,7 +62,6 @@
     list_listinfo(mlist, language)
 
 
-

 def listinfo_overview(msg=''):
     # Present the general listinfo overview
     hostname = Utils.get_domain()
@@ -152,7 +151,6 @@
     print doc.Format()
 
 
-

 def list_listinfo(mlist, lang):
     # Generate list specific listinfo
     doc = HeadlessDocument()
@@ -189,10 +187,25 @@
 
     # If activated, the content and presence of questions and a field to answer them. Added by Systers.
     try:
-        if mlist.essay_enabled:
-            replacements['<mm-questions>'] = """<TR>
-            <TD COLSPAN="3">""" + mlist.questions
-            replacements['<mm-essay-box>'] = TextArea('essay', '', rows=8, cols=60).Format() + """        </TD></TR>"""# Added to support subscribe essays
+        if mlist.essay_enabled: # Added to support subscribe essays
+            html_questions = [] 
+            for question in mlist.questions:
+                question_name = "'"+question[1]+"'"
+                html_questions.append("""<TR> 
+                <TD COLSPAN="3">""" + question[0] + """</TD>""")
+                if (question[2] == 'smallbox'):
+                    html_questions.append("""<TD>""" + TextArea(question_name,\
+                            '', rows=1, cols=60).Format() + """</TD></TR>""") 
+                elif (question[2] == 'largebox'):
+                    html_questions.append("""<TD>""" + TextArea(question_name, \
+                            '', rows=8, cols=60).Format() + """</TD></TR>""") 
+                elif (question[2] == 'checkbox'):
+                    #TODO (Ana Cutillas): consider creating a class like TextArea for checkboxes?
+                    html_questions.append("""<TD><input type='checkbox' name='""" \
+                            + question[1] + """' value =' """\
+                            + question[1] + """'></TD></TR>""") 
+            replacements['<mm-questions>'] = ''.join(html_questions)
+            replacements['<mm-essay-box>'] = ''
         else:
             replacements['<mm-questions>'] = ''
             replacements['<mm-essay-box>'] = ''

=== modified file 'Mailman/Cgi/subscribe.py'
--- Mailman/Cgi/subscribe.py	2009-08-14 15:41:17 +0000
+++ Mailman/Cgi/subscribe.py	2012-08-04 00:41:20 +0000
@@ -26,6 +26,7 @@
 from Mailman import MailList
 from Mailman import Errors
 from Mailman import i18n
+from Mailman import DlistUtils # to write to the database
 from Mailman import Message
 from Mailman.DlistUserDesc import DlistUserDesc # to support subscribe essays
 from Mailman.htmlformat import *
@@ -137,12 +138,36 @@
 
     # Added to support mailing lists that require essays to join
     if mlist.essay_enabled:
-        essay = cgidata.getvalue('essay', '')
-        if not essay:    # Check for empty essays.
-            if not error_msg:
-                print_error(mlist, doc)
-                error_msg = True
-            results.append(_('To subscribe to this list, you must answer the questions asked directly above the large text box.'))
+        # We need to retrieve the questions to be able to 
+        # retrieve the content from cgi.
+        essay = []
+        # This implementation allows for checkboxes to be left unchecked but text boxes must be filled.
+        for question in mlist.questions:
+            # A question is a tuple that contains this information:
+            # (question, question_name, type_of_box, format_info, position, version)
+            answer = cgidata.getvalue(question[1], '')
+            if (answer):
+                # The question has been answered
+                if (len(answer) <= 1500):
+                    # In the database we have set the essay max length to 1500
+                    essay.append((question[1], answer, question[5]))
+                else:
+                    if not error_msg:
+                        print_error(mlist, doc)
+                        error_msg = True
+                    results.append(_('At least one of your answers is too long. Your answers must be less than 1500 characters long.'))
+
+            else:
+                if (question[2] != 'checkbox'):
+                    # If the answer is empty and it's not a checkbox
+                    # the user left fields empty
+                    if not error_msg:
+                        print_error(mlist, doc)
+                        error_msg = True
+                    results.append(_('To subscribe to this list, you must answer all the questions asked on this page.'))
+                else:
+                    # If the answer is an empty checkbox, the user didn't check it
+                    essay.append((question[1], 'False', question[5]))
     else:      # has to be here, else error message when calling DlistUserDesc
         essay = cgidata.getvalue('essay', 'None')
     
@@ -215,6 +240,11 @@
 may have to be first confirmed by you via email, or approved by the list
 moderator.  If confirmation is required, you will soon get a confirmation
 email which contains further instructions.""")
+    for answer in essay:
+        form = DlistUtils.FormQuestion(mlist)
+        form_question_id = form.recordAnswer(email, answer[0], answer[1], answer[2])  
+        # If form_question_id is -1, the user has a pending subscription 
+        # An appropriate message is giving to the user later
 
     try:
         userdesc = DlistUserDesc(email, fullname, password, digest, lang, essay)
@@ -303,6 +333,11 @@
         else:
             results = _("""\
 You have been successfully subscribed to the %(realname)s mailing list.""")
+    if (form_question_id == -1):
+        if not error_msg:
+            print_error(mlist, doc)
+            error_msg = True
+        results = _("""There is a pending subscription with this email.""")
     # Show the results
     ##  Not sure why this is needed
     if results == []:

=== modified file 'Mailman/DlistUtils.py'
--- Mailman/DlistUtils.py	2010-01-31 20:29:48 +0000
+++ Mailman/DlistUtils.py	2012-08-04 00:41:20 +0000
@@ -289,6 +289,53 @@
         self.store.find(Alias,Alias.subscriber_id == obsolete_id).set(subscriber_id = new_id)
         self.store.find(Subscriber,Subscriber.subscriber_id == obsolete_id).remove()    
 
+class FormQuestion(object):
+    __storm_table__ = "form_question"
+    form_question_id = Int(primary = True,default = AutoReload)
+    content = Unicode()
+    user_email = Unicode()
+    version = Int()
+    question_id = Unicode()
+    status = Unicode()
+    
+    def __init__(self,mlist):
+        self.mlist = mlist
+        self.database = getConn(mlist)
+
+    @decfunc
+    def recordAnswer(self, addr, question, answer, version):
+        """Save the answer for a question of the subscribe form, returning its id."""
+        subscriber = Subscriber(self.mlist)
+        userID = addr.decode('utf-8')
+        question_unicode = question.decode('utf-8')
+        count = (self.store.find(FormQuestion, FormQuestion.user_email == userID, FormQuestion.question_id == question_unicode)).count()
+        if count:
+            # The user has already tried to subscribe. 
+            return -1
+        status = u"pending"
+        question_unicode = question.decode('utf-8')
+        answer_unicode = answer.decode('utf-8')
+        #form_question_id has autoreload set, its value will be serially updated in database
+        self.user_email = userID
+        self.content = answer_unicode
+        self.version = version
+        self.question_id = question_unicode
+        self.status = status
+        self.store.add(self)
+        formQuestionID = self.store.find(FormQuestion).max(FormQuestion.form_question_id)
+        if DEBUG_MODE:
+            syslog('info', 'Result of query(formQuestionID) is: %s\n', formQuestionID)
+        return formQuestionID
+
+    @decfunc
+    def acceptAnswer(self, addr):
+        #TODO (Ana Cutillas): Finish this function if we need it. 
+        """Sets the status of the answers to accepted"""
+        subscriber = Subscriber(self.mlist)
+        userID = subscriber.getSubscriber_id_raw_or_die(addr)
+#        command = "form_question = self.store.
+
+
 class Message(object):
     __storm_table__ = "message"
     message_id = Int(primary = True,default = AutoReload)

=== modified file 'bin/rmlist'
--- bin/rmlist	2009-07-31 10:49:01 +0000
+++ bin/rmlist	2012-08-04 00:41:20 +0000
@@ -152,12 +152,7 @@
 
     # Remove the database in PostgreSQL if it was a dynamic list. Added by Systers.
     try:
-        conn = DlistUtils.getConn(None)
-        old_iso_lvl = conn.isolation_level
-        conn.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
-        cursor = conn.cursor()
-        cursor.execute('DROP DATABASE "%s"' % listname)
-        conn.set_isolation_level(old_iso_lvl)
+        DlistUtils.remove_dlist(listname)
     except:
         syslog('error', 'Tried to remove the database for %s, no such database (ok if it was a non dlist). If it was a dlist, the postgreSQL database for this list needs to be removed manually.' % listname)