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

Proposed by Ana Cutillas
Status: Superseded
Proposed branch: lp:~acoconut/systers/flexible_essays
Merge into: lp:systers
Diff against target: 176 lines (+80/-12)
4 files modified
Mailman/Cgi/listinfo.py (+24/-6)
Mailman/Cgi/subscribe.py (+10/-0)
Mailman/DlistUtils.py (+45/-0)
bin/rmlist (+1/-6)
To merge this branch: bzr merge lp:~acoconut/systers/flexible_essays
Reviewer Review Type Date Requested Status
Robin J Approve
beachbrake Pending
Review via email: mp+112665@code.launchpad.net

This proposal supersedes a proposal from 2012-06-27.

Description of the change

I changed the name of the variable that holds the html questions from "questions" to "html_questions".

To post a comment you must log in.
Revision history for this message
Robin J (robin-jeffries) wrote : Posted in a previous version of this proposal

A suggestion for the future: when you put in a syslog statement, put in some identifying text, preferably the file that the message came from and what the data is. e.g.,

 syslog('info', "listinfo.py: essay question data for question %d = %s", i, mlist.questions[i][i+1][2])

In listinfo.py

I'm confused by your parameters --can you remind me what the datastructure is for mlist.questions (what the columns are? I don't understand how you can do things with mlist.questions[i][i+1][0] etc. I think this must have worked "by accident". )

I believe that mailman coding style requires 80 column line lengths. Can you shorten the long lines? (you may need to add some extra parens to make python understand that your new lines are continuation lines).

For things you are planning to do later (even if later is in the indefinite future, please start the comment with:
#TODO (your name):
that way it's easy to grep the code and find things you left for later.

As best I can tell, you didn't actually make any changes to DlistUtils.py. Is that correct? If not, can you point out the places you made changes (it looks like these were the changes to fix our nasty lower case bug; maybe you had to import that file onto your branch?)

Where do you define mlist.questions? (presumably in MailList.py, but why wasn't that included in your merge?) If you just manually created a mailing list with questions, please put that in the description of the change, showing exactly what is in that faked data structure.

Use the description of the change to tell your reviewers the things they need to know to understand this code (like the mlist.questions data structure)

review: Needs Fixing
Revision history for this message
Ana Cutillas (acoconut) wrote : Posted in a previous version of this proposal

> Review: Needs Fixing
>
> A suggestion for the future: when you put in a syslog statement, put in
> some identifying text, preferably the file that the message came from and
> what the data is. e.g.,
>
> syslog('info', "listinfo.py: essay question data for question %d = %s",
> i, mlist.questions[i][i+1][2])
>

Mmm... I thought I had gotten rid of all the syslog statements, I was just
using them for my own testing purposes.

>
> In listinfo.py
>
> I'm confused by your parameters --can you remind me what the datastructure
> is for mlist.questions (what the columns are? I don't understand how you
> can do things with mlist.questions[i][i+1][0] etc. I think this must have
> worked "by accident". )
>

[{1: ['talk about yourself', 'talk', 'largebox', 300, 'top']}, {2: ['Are
you a woman?', 'woman', 'checkbox', 0, 'bottom']}]

So basically:

[{question_number: ['question description', 'short version of question',
'type of box', 'size of box', 'position']}]

Why do you think this works by accident?

>
> I believe that mailman coding style requires 80 column line lengths. Can
> you shorten the long lines? (you may need to add some extra parens to make
> python understand that your new lines are continuation lines).
>

Ok, I will. There are some files in the systers folders that have very long
lines too, should I shorten those too? I mean code that I didn't write but
has very long lines.

>
> For things you are planning to do later (even if later is in the
> indefinite future, please start the comment with:
> #TODO (your name):
> that way it's easy to grep the code and find things you left for later.
>
> As best I can tell, you didn't actually make any changes to DlistUtils.py.
> Is that correct? If not, can you point out the places you made changes
> (it looks like these were the changes to fix our nasty lower case bug;
> maybe you had to import that file onto your branch?)
>

I only modified listinfo.py for the changes for the first week.

>
> Where do you define mlist.questions? (presumably in MailList.py, but why
> wasn't that included in your merge?) If you just manually created a
> mailing list with questions, please put that in the description of the
> change, showing exactly what is in that faked data structure.
>

mlist.questions was something that mailman was already using. It used to
get the one question from the pickle file, now it gets the data structure I
posted earlier.

> Use the description of the change to tell your reviewers the things they
> need to know to understand this code (like the mlist.questions data
> structure)
>
>
> Please, let me know if all this made or not sense and I'll make the
changes and resubmit .

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

Revision history for this message
Robin J (robin-jeffries) wrote : Posted in a previous version of this proposal

1. It's fine to leave the syslog statements in (you can make a grep for them at the end of the summer). If they happen to make it into production, we turn DEBUG off there, so there is no harm. And you never know when a bug will come back to you

2. For the way you march down the mlist.questions data structure, I don't understand how you got it to work. If I understand it correctly, given

[{1: ['talk about yourself', 'talk', 'largebox', 300, 'top']}, {2: ['Are
you a woman?', 'woman', 'checkbox', 0, 'bottom']}]

your tests are for mlist.questions[i][i+1][2]

the first time through that is mlist.questions[0][1][2] (in line 15 in the diff) -- that would be 'talk' above.)

The next time through it is mlist.questions[1][2][2] -- which doesn't exist, right? shouldn't your test (and other items) be mlist.questions[i][1][2]?

(also, I missed this earlier. It would be better code to have your loop statement in line 12 in the diff be
  for question in mlist.questions:
(range has to take that number an convert it to a location in the data structure, and this just marches down the data structure)

If you do that, then the test becomes something like
if question[1][2] ==...

(see how this is clearer? You could make it even clearer by making this a named structure (where you might have question.boxtype for the above), but for the small number of times you are going to reference it, it may not be worth it. But we may want to revisit this when we see how complex the admin UI is and how much improvement in clarity it gives)

3. For the updating of mlist.questions. I understand more now, but I think there is going to be an issue when you do the admin GUI -- this is not where things that are changed in the admin gui are stored (they are not part of mlist). This is not something you introduced, but it is something you will need to correct when you get to doing the admin GUI for this. Let's leave that issue until later.

Revision history for this message
Ana Cutillas (acoconut) wrote : Posted in a previous version of this proposal

Yes, I see what you meant about the for loop and I just got done fixing it.
I had to add an index though, I'm going to push the new version so that you
can see it. I couldn't figure out a way of doing it without an index.

As for the lines length, there are lines a lot longer that I didn't write.
I made mine shorter, let me know if I should change the ones I didn't write
too.

Thanks Robin! I love learning little things like the for loop thing. :)

On Tue, Jun 26, 2012 at 7:26 AM, Robin J <email address hidden> wrote:

> 1. It's fine to leave the syslog statements in (you can make a grep for
> them at the end of the summer). If they happen to make it into production,
> we turn DEBUG off there, so there is no harm. And you never know when a
> bug will come back to you
>
> 2. For the way you march down the mlist.questions data structure, I don't
> understand how you got it to work. If I understand it correctly, given
>
> [{1: ['talk about yourself', 'talk', 'largebox', 300, 'top']}, {2: ['Are
> you a woman?', 'woman', 'checkbox', 0, 'bottom']}]
>
> your tests are for mlist.questions[i][i+1][2]
>
> the first time through that is mlist.questions[0][1][2] (in line 15 in
> the diff) -- that would be 'talk' above.)
>
> The next time through it is mlist.questions[1][2][2] -- which doesn't
> exist, right? shouldn't your test (and other items) be
> mlist.questions[i][1][2]?
>
> (also, I missed this earlier. It would be better code to have your loop
> statement in line 12 in the diff be
> for question in mlist.questions:
> (range has to take that number an convert it to a location in the data
> structure, and this just marches down the data structure)
>
> If you do that, then the test becomes something like
> if question[1][2] ==...
>
> (see how this is clearer? You could make it even clearer by making this a
> named structure (where you might have question.boxtype for the above), but
> for the small number of times you are going to reference it, it may not be
> worth it. But we may want to revisit this when we see how complex the
> admin UI is and how much improvement in clarity it gives)
>
> 3. For the updating of mlist.questions. I understand more now, but I
> think there is going to be an issue when you do the admin GUI -- this is
> not where things that are changed in the admin gui are stored (they are not
> part of mlist). This is not something you introduced, but it is something
> you will need to correct when you get to doing the admin GUI for this.
> Let's leave that issue until later.
>
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/111405
> You are the owner of lp:~acoconut/systers/flexible_essays.
>

Revision history for this message
Ana Cutillas (acoconut) wrote : Posted in a previous version of this proposal
Download full text (29.7 KiB)

Mm, ignore the import of DlistUtils for revision, I accidentally left it
there from when I was trying to make the STORM stuff work. Sorry!

On Tue, Jun 26, 2012 at 7:46 PM, 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)
>
> For more details, see:
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112178
>
> I fixed everything Robin told me too:
>
> - The for loop that goes through the list of questions.
> - The length of the lines I wrote.
> - Deleted the syslog statements I wrote when I was debugging because they
> weren't really useful.
> - Added a TODO line.
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112178
> 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-06-26 17:45:29 +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()
> @@ -190,9 +188,24 @@
> # 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
> + questions = [] # Added to support subscribe essays
> + index = 1
> + for question in mlist.questions:
> + questions.append("""<TR>
> + <TD COLSPAN="3">""" + question[index][0] + """</TD>""")
> + if (question[index][2] == 'smallbox'):
> + questions.append("""<TD>""" +
> TextArea(question[index][1],\
> + '', rows=1, cols=60).Format() +
> """</TD></TR>""")
> + elif (question[index][2] == 'largebox'):
> + questions.append("""<TD>""" +
> TextArea(question[index][1], \
> + '', rows=8, cols=60).Format() +
> """</TD></TR>""")
> + elif (question[index][2] == 'checkbox'):
> + #TODO (Ana Cutillas): consider creating a class like
> TextArea for checkboxes?
> + questions.append("""<TD><input type='checkbox'
> name='""" + \
> + question[index][1] + """' value ='""" + \
> + ...

Revision history for this message
Robin J (robin-jeffries) wrote : Posted in a previous version of this proposal
Download full text (3.2 KiB)

That's what code reviews are for.

I will look at the changed code as soon as I can, but I have out of town
guests tonight, and I don't know if I can steal some cycles from work or
not.

Robin

On Tue, Jun 26, 2012 at 10:34 AM, Ana Cutillas <email address hidden> wrote:

> Yes, I see what you meant about the for loop and I just got done fixing it.
> I had to add an index though, I'm going to push the new version so that you
> can see it. I couldn't figure out a way of doing it without an index.
>
> As for the lines length, there are lines a lot longer that I didn't write.
> I made mine shorter, let me know if I should change the ones I didn't write
> too.
>
> Thanks Robin! I love learning little things like the for loop thing. :)
>
> On Tue, Jun 26, 2012 at 7:26 AM, Robin J <email address hidden> wrote:
>
> > 1. It's fine to leave the syslog statements in (you can make a grep for
> > them at the end of the summer). If they happen to make it into
> production,
> > we turn DEBUG off there, so there is no harm. And you never know when a
> > bug will come back to you
> >
> > 2. For the way you march down the mlist.questions data structure, I
> don't
> > understand how you got it to work. If I understand it correctly, given
> >
> > [{1: ['talk about yourself', 'talk', 'largebox', 300, 'top']}, {2: ['Are
> > you a woman?', 'woman', 'checkbox', 0, 'bottom']}]
> >
> > your tests are for mlist.questions[i][i+1][2]
> >
> > the first time through that is mlist.questions[0][1][2] (in line 15 in
> > the diff) -- that would be 'talk' above.)
> >
> > The next time through it is mlist.questions[1][2][2] -- which doesn't
> > exist, right? shouldn't your test (and other items) be
> > mlist.questions[i][1][2]?
> >
> > (also, I missed this earlier. It would be better code to have your loop
> > statement in line 12 in the diff be
> > for question in mlist.questions:
> > (range has to take that number an convert it to a location in the data
> > structure, and this just marches down the data structure)
> >
> > If you do that, then the test becomes something like
> > if question[1][2] ==...
> >
> > (see how this is clearer? You could make it even clearer by making this
> a
> > named structure (where you might have question.boxtype for the above),
> but
> > for the small number of times you are going to reference it, it may not
> be
> > worth it. But we may want to revisit this when we see how complex the
> > admin UI is and how much improvement in clarity it gives)
> >
> > 3. For the updating of mlist.questions. I understand more now, but I
> > think there is going to be an issue when you do the admin GUI -- this is
> > not where things that are changed in the admin gui are stored (they are
> not
> > part of mlist). This is not something you introduced, but it is
> something
> > you will need to correct when you get to doing the admin GUI for this.
> > Let's leave that issue until later.
> >
> > --
> >
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/111405
> > You are the owner of lp:~acoconut/systers/flexible_essays.
> >
>
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/111405
> You are reviewing the pro...

Read more...

Revision history for this message
Robin J (robin-jeffries) wrote : Posted in a previous version of this proposal
Download full text (32.0 KiB)

Ana, please test things before you send out the code for review.

And also add priya and me both to all the reviews. We will trade off who
is reviewing things, but it's good for both of us to see the others
comments.

On Tue, Jun 26, 2012 at 10:48 AM, Ana Cutillas <email address hidden> wrote:

> Mm, ignore the import of DlistUtils for revision, I accidentally left it
> there from when I was trying to make the STORM stuff work. Sorry!
>
> On Tue, Jun 26, 2012 at 7:46 PM, 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)
>>
>> For more details, see:
>> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112178
>>
>> I fixed everything Robin told me too:
>>
>> - The for loop that goes through the list of questions.
>> - The length of the lines I wrote.
>> - Deleted the syslog statements I wrote when I was debugging because they
>> weren't really useful.
>> - Added a TODO line.
>> --
>> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112178
>> 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-06-26 17:45:29 +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()
>> @@ -190,9 +188,24 @@
>> # 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
>> + questions = [] # Added to support subscribe essays
>> + index = 1
>>
>
No, you don't need index; actually, index should always be 1

 for the data structure

[{1: ['talk about yourself', 'talk', 'largebox', 300, 'top']}, {2: ['Are
you a woman?', 'woman', 'checkbox', 0, 'bottom']}]

the first time through, question is
{1: ['talk about yourself', 'talk', 'largebox', 300, 'top']}
and the second time through it is
{2: ['Are you a woman?', 'woman', 'checkbox', 0, 'bottom']}

To address 'largebox' and 'checkbox' you want
question[1][2] not
question[index][2]

(If you had tested your code, you would have seen that this causes problems)

(for now, I...

Revision history for this message
Ana Cutillas (acoconut) wrote : Posted in a previous version of this proposal
Download full text (34.4 KiB)

Mmm... I definitely tested it before submitting and I actually went back to
it right now and it is working in my environment. AND when I change index
for 1, it doesn't work. I even opened a python interpreter earlier today
and tested the for loop without index and with index and it wouldn't work
with index. I can write up my testes again and show them to you.

It's almost 3am right now (but 82ºF (28 ºC) - I am a Spaniard but heat...
it kills me. I don´t know if I'll ever be able to handle it.) but I will
try the data structure you proposed tomorrow and let you know how that
goes. You're right that I'm not really doing anything with the question
numbers.

On Tue, Jun 26, 2012 at 11:58 PM, Robin J <email address hidden> wrote:

> Ana, please test things before you send out the code for review.
>
> And also add priya and me both to all the reviews. We will trade off who
> is reviewing things, but it's good for both of us to see the others
> comments.
>
>
> On Tue, Jun 26, 2012 at 10:48 AM, Ana Cutillas <email address hidden>
> wrote:
>
> > Mm, ignore the import of DlistUtils for revision, I accidentally left it
> > there from when I was trying to make the STORM stuff work. Sorry!
> >
> > On Tue, Jun 26, 2012 at 7:46 PM, 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)
> >>
> >> For more details, see:
> >>
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112178
> >>
> >> I fixed everything Robin told me too:
> >>
> >> - The for loop that goes through the list of questions.
> >> - The length of the lines I wrote.
> >> - Deleted the syslog statements I wrote when I was debugging because
> they
> >> weren't really useful.
> >> - Added a TODO line.
> >> --
> >>
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112178
> >> 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-06-26 17:45:29 +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()
> >> @@ -190,9 +188,24 @@
> >> # 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>
> >> - ...

Revision history for this message
Robin J (robin-jeffries) wrote : Posted in a previous version of this proposal

OK, we are definitely making progress. Some questions and one change request:
   - what do fields 3 and 4 mean in your data structure (0, and 'bottom' for the checkbox?)
   - I'm not sure what you are doing for the checkbox (because I don't really understand that html). The part that confuses me is that you are using question[1] there twice. One part I think you are naming the checkbox, but why the second time? It seems to also be the value?

My requested change is that questions is too similar to mlist.questions. I think the code will be much easier for someone else to read if you change that to something like html_questions

Revision history for this message
Ana Cutillas (acoconut) wrote : Posted in a previous version of this proposal

On Thu, Jun 28, 2012 at 6:51 AM, Robin J <email address hidden> wrote:

> OK, we are definitely making progress. Some questions and one change
> request:
> - what do fields 3 and 4 mean in your data structure (0, and 'bottom'
> for the checkbox?)
>

I am not really doing anything with them yet. They are there for when I
start adding options to make the forms beautiful and let the admin move the
boxes around. Field 3 is the size of the box, a checkbox would look the
same with size 3000 or 0, so we can leave it to 0. Field 4 is the relative
position, I haven't really given it a lot of thought yet, I am currently
printing them in the order they appear in the list. Do you think it would
be better if I took those fields out of the data structure for now if I'm
not using them yet?

  - I'm not sure what you are doing for the checkbox (because I don't
> really understand that html). The part that confuses me is that you are
> using question[1] there twice. One part I think you are naming the
> checkbox, but why the second time? It seems to also be the value?
>
> According to what I read on the internet, there are two fields in the html
syntax to add checkboxes: name and value. Name is just the name of the
checkbox, and value is the value that is submitted if the checkbox is
checked. That's why I'm using the same string for both.

So the HTML code for a checkbox would look like this:

<TD><input type='checkbox' name='woman' value='woman'></TD>

> My requested change is that questions is too similar to mlist.questions.
> I think the code will be much easier for someone else to read if you
> change that to something like html_questions
>

Ok, changed! Let me know what you think about the extra 3 and 4 fields so I
can either change that or resubmit this again.

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

Revision history for this message
Robin J (robin-jeffries) wrote : Posted in a previous version of this proposal

On Thu, Jun 28, 2012 at 3:41 AM, Ana Cutillas <email address hidden> wrote:

> On Thu, Jun 28, 2012 at 6:51 AM, Robin J <email address hidden> wrote:
>
> > OK, we are definitely making progress. Some questions and one change
> > request:
> > - what do fields 3 and 4 mean in your data structure (0, and 'bottom'
> > for the checkbox?)
> >
>
> I am not really doing anything with them yet. They are there for when I
> start adding options to make the forms beautiful and let the admin move the
> boxes around. Field 3 is the size of the box, a checkbox would look the
> same with size 3000 or 0, so we can leave it to 0. Field 4 is the relative
> position, I haven't really given it a lot of thought yet, I am currently
> printing them in the order they appear in the list. Do you think it would
> be better if I took those fields out of the data structure for now if I'm
> not using them yet?
>

No, feel free to keep them, since it doesn't change the way you interact
with the rest of the data structure. My guess is that you may need to
change some of them (probably a relative position than an absolute one),
but that can happen later.

>
> - I'm not sure what you are doing for the checkbox (because I don't
> > really understand that html). The part that confuses me is that you are
> > using question[1] there twice. One part I think you are naming the
> > checkbox, but why the second time? It seems to also be the value?
> >
> > According to what I read on the internet, there are two fields in the
> html
> syntax to add checkboxes: name and value. Name is just the name of the
> checkbox, and value is the value that is submitted if the checkbox is
> checked. That's why I'm using the same string for both.
>
> So the HTML code for a checkbox would look like this:
>
> <TD><input type='checkbox' name='woman' value='woman'></TD>
>

OK, get it now. I can't see any harm in using the same value for both of
them, but you might keep this in mind if you run into a bug -- that the
wrong field maybe is being addressed.

>
> > My requested change is that questions is too similar to mlist.questions.
> > I think the code will be much easier for someone else to read if you
> > change that to something like html_questions
> >
>
> Ok, changed! Let me know what you think about the extra 3 and 4 fields so I
> can either change that or resubmit this again.
>
> Go for it.

>
> > --
> >
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112331
> > You are the owner of lp:~acoconut/systers/flexible_essays.
> >
>
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112331
> You are requested to review the proposed merge of
> lp:~acoconut/systers/flexible_essays into lp:systers.
>

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

LGTM (That's google speak for "Looks good to me"). I'd like to have beachbrake take a look over this, for her insights, but you can move on to other things. I'm pretty sure we have hit the big stuff here.

review: Approve
Revision history for this message
beachbrake (wordsofagirl) wrote :
Download full text (30.0 KiB)

Looks clean to me too. Go ahead.

- Priya

On Fri, Jun 29, 2012 at 4:15 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/112665<https://code.launchpad.net/%7Eacoconut/systers/flexible_essays/+merge/112665>
>
> I changed the name of the variable that holds the html questions from
> "questions" to "html_questions".
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/112665<https://code.launchpad.net/%7Eacoconut/systers/flexible_essays/+merge/112665>
> You are requested to review the proposed merge of
> lp:~acoconut/systers/flexible_essays into lp:systers.
>
> === modified file 'Mailman/Cgi/listinfo.py'
> --- Mailman/Cgi/listinfo.py 2009-08-14 15:41:17 +0000
> +++ Mailman/Cgi/listinfo.py 2012-06-28 22:44:19 +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:
> @@ -53,6 +53,8 @@
> syslog('error', 'No such list "%s": %s', listname, e)
> return
>
> + ##acoconut formquestion = DlistUtils.FormQuestion(mlist)
> + ##acoconut formquestion.recordAnswer("<email address hidden>", 1, "yes,
> i am", 1)
> # See if the user want to see this page in other language
> cgidata = cgi.FieldStorage()
> language = cgidata.getvalue('language')
> @@ -62,7 +64,6 @@
> list_listinfo(mlist, language)
>
>
> -
> def listinfo_overview(msg=''):
> # Present the general listinfo overview
> hostname = Utils.get_domain()
> @@ -152,7 +153,6 @@
> print doc.Format()
>
>
> -
> def list_listinfo(mlist, lang):
> # Generate list specific listinfo
> doc = HeadlessDocument()
> @@ -190,9 +190,22 @@
> # 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
> + html_questions = [] # Added to support subscribe essays
> + for question in mlist.questions:
> + html_questions.append("""<TR>
> + <TD COLSPAN="3">""" + question[0] + """</TD>""")
> + if (question[2] == 'smallbox'):
> + html_questions.append("""<TD>""" +
> TextArea(question[1],\
> + '', rows=1, cols=60).Format() +
> """</TD></TR>""")
> + elif (question[2] == 'largebox'):
> + html_questions.append("""<TD>""" +
> TextArea(question[1], \
> + '', rows=8, cols=60).Format() +
> """</TD></TR...

lp:~acoconut/systers/flexible_essays updated
87. By Ana Cutillas <email address hidden>

Added changes to the DlistUtils.py file to handle the database
operations to the new table form_question.

88. By Ana Cutillas <email address hidden>

Added the database backend and the functionality to connect forms to the database.

89. By Ana Cutillas <email address hidden>

Made changes to the database and to the way we store the answers. Made
changes to the way we handle the information coming from the form.

90. By Ana Cutillas <email address hidden>

Cleaned up listinfo.py

91. By Ana Cutillas <email address hidden>

Cleaned up.

92. By Ana Cutillas <email address hidden>

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

93. By Ana Cutillas <email address hidden>

Removed the unused function get_answer from DlistUtils.py

94. By Ana Cutillas <email address hidden>

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>

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

96. By Ana Cutillas <email address hidden>

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>

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>

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

99. By Ana Cutillas <email address hidden>

Added support for setting answers to rejected in the database.

Unmerged revisions

99. By Ana Cutillas <email address hidden>

Added support for setting answers to rejected in the database.

98. By Ana Cutillas <email address hidden>

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

97. By Ana Cutillas <email address hidden>

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>

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>

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

94. By Ana Cutillas <email address hidden>

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>

Removed the unused function get_answer from DlistUtils.py

92. By Ana Cutillas <email address hidden>

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

91. By Ana Cutillas <email address hidden>

Cleaned up.

90. By Ana Cutillas <email address hidden>

Cleaned up listinfo.py

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Mailman/Cgi/listinfo.py'
2--- Mailman/Cgi/listinfo.py 2009-08-14 15:41:17 +0000
3+++ Mailman/Cgi/listinfo.py 2012-07-25 19:01:19 +0000
4@@ -22,6 +22,7 @@
5 import os
6 import cgi
7
8+from Mailman import DlistUtils
9 from Mailman import mm_cfg
10 from Mailman import Utils
11 from Mailman import MailList
12@@ -36,7 +37,6 @@
13 i18n.set_language(mm_cfg.DEFAULT_SERVER_LANGUAGE)
14
15
16-
17
18 def main():
19 parts = Utils.GetPathPieces()
20 if not parts:
21@@ -53,6 +53,8 @@
22 syslog('error', 'No such list "%s": %s', listname, e)
23 return
24
25+ ##acoconut formquestion = DlistUtils.FormQuestion(mlist)
26+ ##acoconut formquestion.recordAnswer("coconut@coconut.net", 1, "yes, i am", 1)
27 # See if the user want to see this page in other language
28 cgidata = cgi.FieldStorage()
29 language = cgidata.getvalue('language')
30@@ -62,7 +64,6 @@
31 list_listinfo(mlist, language)
32
33
34-
35
36 def listinfo_overview(msg=''):
37 # Present the general listinfo overview
38 hostname = Utils.get_domain()
39@@ -152,7 +153,6 @@
40 print doc.Format()
41
42
43-
44
45 def list_listinfo(mlist, lang):
46 # Generate list specific listinfo
47 doc = HeadlessDocument()
48@@ -190,9 +190,27 @@
49 # If activated, the content and presence of questions and a field to answer them. Added by Systers.
50 try:
51 if mlist.essay_enabled:
52- replacements['<mm-questions>'] = """<TR>
53- <TD COLSPAN="3">""" + mlist.questions
54- replacements['<mm-essay-box>'] = TextArea('essay', '', rows=8, cols=60).Format() + """ </TD></TR>"""# Added to support subscribe essays
55+ html_questions = [] # Added to support subscribe essays
56+ index = 0
57+ for question in mlist.questions:
58+ box_name = "essay.q"+str(index)
59+ syslog('info', "Box name: %s", box_name)
60+ index = index + 1
61+ html_questions.append("""<TR>
62+ <TD COLSPAN="3">""" + question[0] + """</TD>""")
63+ if (question[2] == 'smallbox'):
64+ html_questions.append("""<TD>""" + TextArea("essay",\
65+ '', rows=1, cols=60).Format() + """</TD></TR>""")
66+ elif (question[2] == 'largebox'):
67+ html_questions.append("""<TD>""" + TextArea("essay", \
68+ '', rows=8, cols=60).Format() + """</TD></TR>""")
69+ elif (question[2] == 'checkbox'):
70+ #TODO (Ana Cutillas): consider creating a class like TextArea for checkboxes?
71+ html_questions.append("""<TD><input type='checkbox' name='essay' \
72+ value ='""" + \
73+ question[1] + """'></TD></TR>""")
74+ replacements['<mm-questions>'] = ''.join(html_questions)
75+ replacements['<mm-essay-box>'] = ''
76 else:
77 replacements['<mm-questions>'] = ''
78 replacements['<mm-essay-box>'] = ''
79
80=== modified file 'Mailman/Cgi/subscribe.py'
81--- Mailman/Cgi/subscribe.py 2009-08-14 15:41:17 +0000
82+++ Mailman/Cgi/subscribe.py 2012-07-25 19:01:19 +0000
83@@ -26,6 +26,7 @@
84 from Mailman import MailList
85 from Mailman import Errors
86 from Mailman import i18n
87+from Mailman import DlistUtils # to write to the database
88 from Mailman import Message
89 from Mailman.DlistUserDesc import DlistUserDesc # to support subscribe essays
90 from Mailman.htmlformat import *
91@@ -215,6 +216,15 @@
92 may have to be first confirmed by you via email, or approved by the list
93 moderator. If confirmation is required, you will soon get a confirmation
94 email which contains further instructions.""")
95+ question_index = 0
96+ if (isinstance(essay, str)): #There's only one question, so it comes as a string.
97+ form = DlistUtils.FormQuestion(mlist)
98+ form.recordAnswer(email, question_index, essay, 1)
99+ else:
100+ for answer in essay:
101+ form = DlistUtils.FormQuestion(mlist)
102+ form.recordAnswer(email, question_index, answer, 1)
103+ question_index = question_index + 1
104
105 try:
106 userdesc = DlistUserDesc(email, fullname, password, digest, lang, essay)
107
108=== modified file 'Mailman/DlistUtils.py'
109--- Mailman/DlistUtils.py 2010-01-31 20:29:48 +0000
110+++ Mailman/DlistUtils.py 2012-07-25 19:01:19 +0000
111@@ -289,6 +289,51 @@
112 self.store.find(Alias,Alias.subscriber_id == obsolete_id).set(subscriber_id = new_id)
113 self.store.find(Subscriber,Subscriber.subscriber_id == obsolete_id).remove()
114
115+class FormQuestion(object):
116+ __storm_table__ = "form_question"
117+ form_question_id = Int(primary = True,default = AutoReload)
118+ content = Unicode()
119+ user_email = Unicode()
120+ version = Int()
121+ question_id = Int()
122+ status = Unicode()
123+
124+ def __init__(self,mlist):
125+ self.mlist = mlist
126+ self.database = getConn(mlist)
127+
128+ @decfunc
129+ def recordAnswer(self, addr, question, answer, version):
130+ """Save the answer for a question of the subscribe form, returning its id."""
131+ subscriber = Subscriber(self.mlist)
132+ userID = addr.decode('utf-8')
133+ status = u"pending"
134+ answer_unicode = answer.decode('utf-8')
135+
136+ command = "form_question = self.store.add(FormQuestion())\nform_question.content = answer_unicode\nform_question.email = userID\nform_question.version = version\nform_question.question_id = question\nform_question.status = status\n"
137+ if DEBUG_MODE:
138+ syslog('info', 'DlistUtils:(recordAnswer)executing query:\n%s', command)
139+ syslog('info', 'DlistUtils:(recordAnswer) query parameters:\n content:%s\n userid:%s\n version:%s\n question_id:%s\n status:%s\n ', answer_unicode, userID, version, question, status)
140+ #form_question_id has autoreload set, its value will be serially updated in database
141+ self.user_email = userID
142+ self.content = answer_unicode
143+ self.version = version
144+ self.question_id = question
145+ self.status = status
146+ self.store.add(self)
147+ formQuestionID = self.store.find(FormQuestion).max(FormQuestion.form_question_id)
148+ if DEBUG_MODE:
149+ syslog('info', 'Result of query(formQuestionID) is: %s\n', formQuestionID)
150+ return formQuestionID
151+
152+ def acceptAnswer(self, addr):
153+ #TODO (Ana Cutillas): Finish this function if we need it.
154+ """Sets the status of the answers to accepted"""
155+ subscriber = Subscriber(self.mlist)
156+ userID = subscriber.getSubscriber_id_raw_or_die(addr)
157+# command = "form_question = self.store.
158+
159+
160 class Message(object):
161 __storm_table__ = "message"
162 message_id = Int(primary = True,default = AutoReload)
163
164=== modified file 'bin/rmlist'
165--- bin/rmlist 2009-07-31 10:49:01 +0000
166+++ bin/rmlist 2012-07-25 19:01:19 +0000
167@@ -152,12 +152,7 @@
168
169 # Remove the database in PostgreSQL if it was a dynamic list. Added by Systers.
170 try:
171- conn = DlistUtils.getConn(None)
172- old_iso_lvl = conn.isolation_level
173- conn.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
174- cursor = conn.cursor()
175- cursor.execute('DROP DATABASE "%s"' % listname)
176- conn.set_isolation_level(old_iso_lvl)
177+ DlistUtils.remove_dlist(listname)
178 except:
179 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)
180

Subscribers

People subscribed via source and target branches