Systers-mailman

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

flexible_essays
Merge into stable

Revision history for this message

Robin J (robin-jeffries) wrote on 2012-07-28:

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

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

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

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

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

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

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

So definitely you can't do it that way, 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.
>

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.

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)

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.

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

« Back to merge proposal