Systers-mailman

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

flexible_essays
Merge into stable

Revision history for this message

Ana Cutillas (acoconut) wrote on 2012-07-28:

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 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, since you can't tell whether this
> someone who didn't have a woman parameter or didn't answer yes.
>
> I suspect there is a way to do this by putting in an invisible field that
> you return that tells you which question set you used (which might be the
> version number), but I think the first approach will work OK.
>
>
> 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 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 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.).
> > >
> > >
> > > Alright that's pretty easy.
> > >
> > >
> > >> 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.
> > >
> > >
> > >
> > >> --
> > >>
> > >>
> >
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/116732
> > >> You are the owner of lp:~acoconut/systers/flexible_essays.
> > >>
> > >
> > >
> >
> > --
> >
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/116732
> > You are reviewing the proposed merge of
> > lp:~acoconut/systers/flexible_essays into lp:systers.
> >
>
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/116732
> You are the owner of lp:~acoconut/systers/flexible_essays.
>

On Sat, Jul 28, 2012 at 6:25 AM, Robin J <robin@jeffries.org> wrote:

> On Fri, Jul 27, 2012 at 11:16 AM, Ana Cutillas <anitacorr@gmail.com>
> 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 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, since you can't tell  whether this
> someone who didn't have a woman parameter or didn't answer yes.
>
> I suspect there is a way to do this by putting in an invisible field that
> you return that tells you which question set you used (which might be the
> version number), but I think the first approach will work OK.
>
>
> 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 <robin@jeffries.org> 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 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.).
> > >
> > >
> > > Alright that's pretty easy.
> > >
> > >
> > >> 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.
> > >
> > >
> > >
> > >> --
> > >>
> > >>
> >
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/116732
> > >> You are the owner of lp:~acoconut/systers/flexible_essays.
> > >>
> > >
> > >
> >
> > --
> >
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/116732
> > You are reviewing the proposed merge of
> > lp:~acoconut/systers/flexible_essays into lp:systers.
> >
>
> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/116732
> You are the owner of lp:~acoconut/systers/flexible_essays.
>

« Back to merge proposal