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

[Ana Cutillas]

I added the functionality to show the pending subscriptions with the new types of essays (in admindb.py)

I fixed the bug found when users tried to write an answer longer than what the database holds (and changed the database schema so it holds up to 1500 characters for the essay answers)

I fixed what happens when a user tries to subscribe twice while their application is being reviewed by making changes to the storm function and counting the number of answers with that question ID and email address. We will have to go through this approach again when we have different form versions, because it will probably not work. I also had to modify subscribe.py, check the return value of the function that writes to the database and give an appropriate error message.

[Robin J (robin-jeffries)]

Ahh, now I understand why we can never make sense of the order the questions come in. Could you change it so they are ordered by subscribe time? (maybe we have to add that field? Or does the database know when the item is entered?) I have always considered the ordering to be random, and when you have "saved" some items to process later, they get reordered and you can't tell the old ones from the new ones.

Also change the comment about modifying this to add essays, since it's not an essay any more. Something like "the answers to the questions asked on the subscribe page?

I think you need to rethink the error checking in subscribe.py First, it's going to confuse the next person who reads your code when you say if len(answer)<1500, since that only applies to text boxes - a checkbox can't be that big. So a branch that has different logic for textboxes than checkboxes will make it clearer. Also, this error message

 results.append(_('At least one of your answers is too long. Your answers must be less than 1500 characters long.'))

is not very use friendly. Tell them which question is too long. You already have info about the questions -- use the long text. (Suppose there are 2 checkboxes and one is under 1500 chars, but not by a lot and the other is over. That will drive the subscriber crazy trying to figure out which ones needs to change)

For results = _("""There is a pending subscription with this email.""")

how about adding "You may not subscribe a second time. The moderator will contact you if there are any questions about the answers you gave originally."

Can you look up and see if there is a way to set a given change (e.g., one that has been approved) as the baseline, so that I can look at only new changes. It's hard to review this stuff when the diffs don't give me much information about what you changed since I last looked at it.

[Ana Cutillas (acoconut)]

On Mon, Aug 6, 2012 at 3:23 AM, Robin J <email address hidden> wrote:

> Ahh, now I understand why we can never make sense of the order the
> questions come in. Could you change it so they are ordered by subscribe
> time? (maybe we have to add that field? Or does the database know when
> the item is entered?) I have always considered the ordering to be random,
> and when you have "saved" some items to process later, they get reordered
> and you can't tell the old ones from the new ones.
>

I don't understand what you mean here...

Right now the database stores the form_question_id (PK), the answer, the
email, the question version, the question id (these look like "talk",
"woman", etc.) and the status (accepted, pending, rejected).

And they are stored in the database in the same order they have in the
form. So I think the cgi module puts the answers in a list in the same
order they have in the form. Then, I read them in subscribe.py and put them
in a new list, in the same order. And then, a function in DlistUtils.py is
called for every answer (still in order) and this functions stores the
answers in the database. I have a few mock subscriptions in the database
and they all have the same order.

> Also change the comment about modifying this to add essays, since it's not
> an essay any more. Something like "the answers to the questions asked on
> the subscribe page?
>
> I think you need to rethink the error checking in subscribe.py First,
> it's going to confuse the next person who reads your code when you say if
> len(answer)<1500, since that only applies to text boxes - a checkbox can't
> be that big. So a branch that has different logic for textboxes than
> checkboxes will make it clearer. Also, this error message
>
> results.append(_('At least one of your answers is too long. Your answers
> must be less than 1500 characters long.'))
>
> is not very use friendly. Tell them which question is too long. You
> already have info about the questions -- use the long text. (Suppose there
> are 2 checkboxes and one is under 1500 chars, but not by a lot and the
> other is over. That will drive the subscriber crazy trying to figure out
> which ones needs to change)
>
> For results = _("""There is a pending subscription with this email.""")
>
> how about adding "You may not subscribe a second time. The moderator will
> contact you if there are any questions about the answers you gave
> originally."
>
> Can you look up and see if there is a way to set a given change (e.g., one
> that has been approved) as the baseline, so that I can look at only new
> changes. It's hard to review this stuff when the diffs don't give me much
> information about what you changed since I last looked at it.
>

Yeah, it's hard for me to understand what part of the code you're talking
about too. I keep clicking resubmit proposal, maybe I should just send a
new one? I'll give it a try and let's see what that gives us.

I think I fixed everything you said.

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

[lp:~acoconut/systers/flexible_essays]

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.

[Robin J (robin-jeffries)]

On Tue, Aug 7, 2012 at 3:11 AM, Ana Cutillas <email address hidden> wrote:

> On Mon, Aug 6, 2012 at 3:23 AM, Robin J <email address hidden> wrote:
>
> > Ahh, now I understand why we can never make sense of the order the
> > questions come in. Could you change it so they are ordered by subscribe
> > time? (maybe we have to add that field? Or does the database know when
> > the item is entered?) I have always considered the ordering to be
> random,
> > and when you have "saved" some items to process later, they get reordered
> > and you can't tell the old ones from the new ones.
> >
>
> I don't understand what you mean here...
>

In admindb.py, there is the comment
  # Alphabetical order by email address

This is how the information is shown on the admindb page. So I'm not
talking about the order of the columns but the order of the rows.

It looks like the issue is that the key is the email address (see the sort
statement a bit lower). You would need to keep track of an approximate
submission time and sort by that. Don't worry about that right now, but
put it on your list of things to do after GSOC is over.

[lp:~acoconut/systers/flexible_essays]

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-07 10: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-07 10: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-07 10: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 *
@@ -135,14 +136,42 @@
                             os.environ.get('REMOTE_ADDR',
                                            'unidentified origin'))
 
-    # Added to support mailing lists that require essays to join
+    # Added to support mailing lists that require answers to questions 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 (question[2] != 'checkbox'):
+                    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_error = "Your answer to the question \"" + question[0] + "\" is too long. Your answers must be less than 1500 characters long."
+                        results.append(_(results_error))
+                else:
+                    # The answer is a checked checkbox
+                    essay.append((question[1], "True", question[5]))
+            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 +244,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 +337,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. You may not subscribe a second time. The moderator will contact you if there are any questions about the answers you gave originally.""")
     # 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-07 10:41:20 +0000
@@ -248,6 +248,12 @@
         command = "count = (self.store.find(Subscriber,Subscriber.mailman_key == unicode(key,'utf-8'))).count()\n"
         if DEBUG_MODE:
             syslog('info', 'DlistUtils(subscribeToList):Executing query:\n%s', command)
+
+        unicode_key = unicode(key, "utf-8")
+        count_essay = (self.store.find(FormQuestion, FormQuestion.user_email == unicode_key)).count()
+        for i in range(count_essay):
+            self.store.find(FormQuestion, FormQuestion.user_email == unicode_key, FormQuestion.status == u"pending").set(status = u"accepted")
+
         # First see if member is subscribed with deleted field = false
         count = (self.store.find(Subscriber,Subscriber.mailman_key == unicode(key,"utf-8"))).count()
 
@@ -255,7 +261,7 @@
             command = "self.store.find(Subscriber,Subscriber.mailman_key == unicode(key,'utf-8')).set(deleted = False)"
             if DEBUG_MODE:
                 syslog('info', 'DlistUtils(subscribeToList):Executing query:\n%s', command)
-            self.store.find(Subscriber,Subscriber.mailman_key == unicode(key,"utf-8")).set(deleted = False)
+            self.store.find(Subscriber,Subscriber.mailman_key == unicode_key).set(deleted = False)
         else:
             # format is text-only (1) by default
             command = "subscriber = self.store.add(Subscriber())\nsubscriber.mailman_key = unicode(key,'utf-8')\nsubscriber.preference = 1\nsubscriber.deleted = False\nsubscriber.format = 1\nsubscriber.suppress = 0"
@@ -289,6 +295,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):
+        """Sets the status of the answers to accepted"""
+        userID = addr.decode('utf-8')
+        count = (self.store.find(FormQuestion, FormQuestion.user_email == userID).count())
+        for i in range(count):
+            self.store.find(FormQuestion, FormQuestion.user_email == userID).set(status = u"accepted")
+
+
 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-07 10: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)