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

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

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.

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?

Why is replacements['<mm-essay-box>'] always ''? Shouldn't you just get rid of it?

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.

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.

If form_question_id is autoupdating, then what are you doing in line 144?

In DlistUtils.py, where did you add remove_dlist() I don't see it.

review: Needs Fixing

« Back to merge proposal