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

[Ana Cutillas]

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

for this line
              if (question[2] != 'checkbox'):

This implies that you want to do this for every kind of widget except checkboxes. No, you are doing it for every kind of widget you currently have except checkboxes. Make it clearer for the next person and have it say
        if (question[2] == 'small text' or question[2] == 'large text'):

you also want to rethink your structure a bit more -- you have two tests for not being a checkbox, and you should be able to combine them into 1. (again, my guess is that if we add new widgets that are not texty, you may want to treat them differently, so explicitly list the question types you are using.

If these lines aren't useful to you, delete them; it's no longer accurate since you changed the utf-8
 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)

You defined acceptAnswer, but earlier you have almost identical code, why? Why not call acceptAnswer there?

And why aren't these identical? why do you have
    unicode_key = unicode(key, "utf-8")
in one place and
    userID = addr.decode('utf-8')
in the other

review: Needs Fixing

[Ana Cutillas (acoconut)]

On Thu, Aug 9, 2012 at 6:08 AM, Robin J <email address hidden> wrote:

> Review: Needs Fixing
>
> for this line
> if (question[2] != 'checkbox'):
>
> This implies that you want to do this for every kind of widget except
> checkboxes. No, you are doing it for every kind of widget you currently
> have except checkboxes. Make it clearer for the next person and have it say
> if (question[2] == 'small text' or question[2] == 'large text'):
>
> you also want to rethink your structure a bit more -- you have two tests
> for not being a checkbox, and you should be able to combine them into 1.
> (again, my guess is that if we add new widgets that are not texty, you may
> want to treat them differently, so explicitly list the question types you
> are using.
>
> If these lines aren't useful to you, delete them; it's no longer accurate
> since you changed the utf-8
> 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)
>
> You defined acceptAnswer, but earlier you have almost identical code, why?
> Why not call acceptAnswer there?
>
> And why aren't these identical? why do you have
> unicode_key = unicode(key, "utf-8")
> in one place and
> userID = addr.decode('utf-8')
> in the other
>

Because I had to figure out how to decode strings and found the addr.decode
way and then I saw somebody else had done it using the unicode function.

I changed all the other things you suggested and added the ordering by
subscription time idea to a tasks list so that I remember to do it later.

I'm pushing the new code now.

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

[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-09 17: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-09 17: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-09 17: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], '')
+            # The question has been answered
+            if ((question[2] == 'largebox') or (question[2] == 'smallbox')):
+                if (answer):
+                    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:
+                    # 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.'))
+            elif (question[2] == 'checkbox'):
+                if (answer):
+                    # The answer is a checkbox
+                    essay.append((question[1], "True", question[5]))
+                else:
+                    # The answer is an unchecked checkbox
+                    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-09 17:41:20 +0000
@@ -245,22 +245,17 @@
     def subscribeToList(self, key):
         """Add a member to the subscriber database, or change the record from deleted if it was already present."""
 
-        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")
+        form_question = FormQuestion(self.mlist)
+        form_question.acceptAnswer(key)
+
         # First see if member is subscribed with deleted field = false
-        count = (self.store.find(Subscriber,Subscriber.mailman_key == unicode(key,"utf-8"))).count()
+        count = (self.store.find(Subscriber,Subscriber.mailman_key == unicode_key)).count()
 
         if count:
-            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"
-            if DEBUG_MODE:
-                syslog('info', 'DlistUtils(subscribeToList)Executing query:\n%s', command)
             self.mailman_key = unicode(key,"utf-8")
             self.preference = 1
             self.deleted = False
@@ -289,6 +284,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, FormQuestion.status == u"pending").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-09 17: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)