Denise (![[staff profile]](https://www.dreamwidth.org/img/silk/identity/user_staff.png)
denise) wrote in ![[site community profile]](https://www.dreamwidth.org/img/comm_staff.png)
dw_dev_training2009-08-01 10:34 pm
denise) wrote in dw_dev_training2009-08-01 10:34 pm
Entry tags:
Walkthrough for Bug 1518
This is a walkthrough for how I patched Bug 1518, "Option to get notified by email when you send a message". I wrote up this walkthrough because it's a good example (IMO) of how a developer makes design choices when taking a top-level bug description and sitting down to implement it -- both architectural/code design and user interface design.
1. I start with looking over the bug description. It's in the form of coming from Suggestions, so I also look over the entry it came from to see if there are any good suggestions in the comments. The only thing there is one "as long as it's opt-in" and two wanting to redesign the entire inbox/messaging structure entirely, so figuring out how to implement this is up to me.
2. I take a look at the compose page. My first thought is that the best way to implement this would be to have a "CC myself" checkbox, that if checked would make a copy of the notice go to the person who sent it. (It could probably even fit nicely above the subject line, right beneath the "To:" field on a separate line.)
3. My next step is to go look at the code for that page and see if I can figure out how I might do it. A quick glance over things tells me that it should be relatively easy: make the checkbox as part of the form that's generated, give the checkbox a value, and if the value is true on submit, do something in lines 63-69 to put the person sending the message into the @to_list variable.
4. Right about now is when I start to think: okay, but what if people will always want to keep a copy of their sent messages in email? It might get annoying if they always have to remember to check the box. We could make the box always checked, and let people who don't want copies of their messages uncheck it, too, but that's got the same problem.
I know from previous bugs that it would be relatively trivial to create a userprop -- "opt_always_cc_mesages" or something like that -- and a new setting in the Account Settings page to govern the setting of it. I also know that it would be pretty trivial to poll that userprop at the time of rendering the PM interface to determine whether or not the checkbox should be checked by default.
But I also know that adding more options like that is not always a good thing. General standards of usability design say that adding more options should be something you do as little as possible: the goal is to pick useful defaults, and so I sit and think about this for a little bit. My personal standard for creating something as an option is, more or less, "do I think more than about 30% of our users are going to want to fiddle around with whatever choice we make for them" with a side helping of "if less than 30% of our users are going to want to fiddle with it, how desperately are they going to want to fiddle with it", and I don't think this is major enough to qualify.
Eventually I decide that the best thing to do is to keep it as an at-the-time-of-composing option, rather than a sitewide option. That narrows the question down to: should the checkbox default to checked or unchecked? Deciding that is going to come down to whether I think that more people are going to want to send themselves CCs of messages they send or not send them, so I poll irc to see what people think. I get a few answers that mostly trend towards "unchecked", and after I go back to the suggestions poll and see how the poll shook out, there are enough people voting "no" or "with changes" on the suggestion that I don't think "checked" should be the right default. (If I'm wrong, I'm certain there'll be a suggestion to change it coming along shortly after the code gets pushed, and I can see how the discussion turns out in that suggestion.)
5. So, that decided, it's time to get down to implementation. I start by opening up inbox/compose.bml and inbox/compose.bml.text. With my workflow, I always like to add the text strings first -- that way I don't forget them -- so I turn to inbox/compose.bml.text and get ready to add a label for the textbox... except the only thing in that file is an error message for what happens when the recipient is suspended, which means that the text for building the form lies elsewhere.
(The inbox and associated text strings are complicated, I've realized.)
So, it's back to the compose page on DW itself, with ?uselang=debug:
http://www.dreamwidth.org/inbox/compose.bml?uselang=debug
...only to discover that most of the compose page isn't stripped to begin with. That distracts me temporarily into going over to Bugzilla and seeing if there's a bug open to English-strip that page. I search on both "English-strip", "strip compose", and "strip inbox" and don't find anything, so I pause and submit Bug 1528 to do that in the future. I won't do the stripping as part of this bug -- it's bad form to combine functional changes like that -- but I will make sure that my changes to the form will be stripped.
So, I add in the text that I'm going to add as
6. My particular workflow tends to be "change the page that displays first, then change the did_post section". (Most pages are combined "what to show someone who first visits the page" and "what to do, and to show someone, after a form has been submitted"; you can find the "after a form has been submitted" in the block that follows an
I do it that way so I can verify that the form is being changed successfully before I try to change what happens when you submit it -- if you do it the other way around, you have to code the whole thing before you can test it, and if you screwed something up and it's working wrong, you have a harder time figuring out what part you screwed up.
So, I go down to that section. I want to stick it before the Userpic selection -- at least, I think I do -- so I stick it right after line 231:
(Notice that I comment my code! It's always a good idea to comment whenever you're adding a new block of logic or whatever, just so that the next person who comes along will know what you did and why you did it.)
I'll also confess that I always forget how to build those checkboxes. Always. A lot of what I do while coding is think "okay, where else in the code is there something else that does something like what this is doing here", and then go look and see how they did it. Since I can never remember the various HTML controls, and they're poorly documented in-code in htmlcontrols.pl, I always wind up looking around via grep.
7. Save and view the page on my hack. Uh-oh:
Looks like I forgot some commas. It should be:
(This is one of the most common mistakes I make, so I figured I'd share it to point out what are good things to look for when you get errors. It's a good idea to start keeping lists of your own most common mistakes, so you can look over what you're coding for those mistakes even before you check them.)
8. Save and reload. The checkbox displays properly now, but the styling on it is weird to my eyes: bold, like the rest of the header items, and the padding on the bottom is a little weird:
It's functional, it's just ugly there. It breaks up the evenness of having the bolded labels on the left and the action items on the right. So, I decide I want to move it down to beneath the Userpic line. I pick up my code and move it down below the line that looks like this:
Reload and .... oh dear:
Looks like having this checkbox outside the closing <div> after $picform is pushing it out to the side. I don't like that, so we're going to have to rearrange things some more.
I can't just take the out of the drop-down userpic line and put it onto the end of the checkbox lines, because the checkbox lines only get called if there's a remote user. But ... that actually makes me wonder; is it even possible to have a situation where there's no remote user viewing the page? Is it possible to message someone while you're logged out? I check the top of the file, and sure enough, there has to be a $remote (remote user), or else the viewer of the page gets redirected to log in.
That means I can safely dispose of the if-block surrounding the section I'm adding, and move the closing of the <div> to that block:
Save and reload, and I get:
I almost like that, but I still don't want it bolded, or quite as big. I change the last line to:
Reload, and I like it now:
9. So, all the styling aside, now it's time to go back and make it so that the box does something when someone hits submit. I go back to the section of code in lines 63-69 that govern recipients, and take another look at it.
I'm pretty sure that all I have to do here is push $remote onto @to_list if the box is checked, like thus:
Save, and now it's time to go test.
10. In order to test private messaging, you have to be running TheSchwartz, and specifically running the worker that deals with messaging. There's a wiki page about getting TheSchwartz set up, but it doesn't quite cover getting it set up on Dreamhacks properly -- I had Sophie's help in getting mine set up, but I can't remember what I did now, so I make a mental note to bug someone to put better instructions in the Wiki for how to set up TheSchwartz on Dreamhacks.
I'm lucky enough to have it already set up, though, so I can just start it up:
I put in a message, hit "send", and get:
Oops.
11. Back to the code. The problem is almost certainly that I didn't put $remote into the right form on that line I added. (I still don't understand the exact this-and-that about what a user object actually is.) I'm going to start off by guessing that $remote is already in whatever form LJ::load_user returns, so when you run it twice -- in the subsequent foreach loop -- it screws something up.
So, I look at the lines before, and see that the other items in the @to_list are usernames, not user objects. Let's try putting the $remote variable into the same form:
Save and try again.
12. This time, the message went through without any errors, which is a plus. I check my inbox, and ... there it is! No email though ... oh, I have the "by email" option checked off for that subscription in my settings. I check it, send the message again, and success! I have a copy of the message in my inbox and in my email.
I also run a negative test: send a message without the box being checked, and make sure I don't get a copy of the message. I don't, so we're good there.
13. ...Except I just realized, doing it this way makes it appear as though you've sent the message to yourself, not to whoever you sent it. (This is because the multiple-message model kinda stinks.)
Now I have a dilemma. Do I try to fix that by pulling out a separate routine to send the email to yourself, with a list of recipients inside the actual email, or do I say that this is good enough and let it go? Advantage to letting it go: I'd be done with the bug. Also, there'd be less code reuse, because the only way I can think to make this work is going to be either to do a lot of refactoring, or to put in another message-sending section, and I'm pretty sure that if I do that, I'm going to have to add another translation string.
The advantages to fixing it, though, outweigh the other advantages: if I leave it like it is, every time I look at it, I'll know that I half-assed this. Dammit. Stupid work ethic.
14. Okay. I go back to the file, and the actual sending of the message is in line 131. The problem is, it's using another function -- LJ::Message->send -- that may actually not be able to do what I want it to do. Let me go and see what it looks like... A structure like that means that the function probably lives in cgi-bin/LJ/, and sure enough, it's in cgi-bin/LJ/Message.pm.
Except I take a look at it, and it's pretty much gibberish to me -- way, way beyond my ability to read, much less hack.
I've got two options here: find someone who does understand the event-sending code to give me a hand, or submit my patch as-is and open up a new bug for someone to fix it up later. At this point, I'm getting tired of working on this bug (and really tired of doing this writeup), and we're having a party tomorrow that I should be helping prep for, so I invoke my fifteen-minute rule: if I can't figure out how to do something in fifteen minutes or less, I pass on it and move along.
So, once this patch gets added, I'm going to open a new bug to fix it. I call this one done, and generate and submit the patch.
1. I start with looking over the bug description. It's in the form of coming from Suggestions, so I also look over the entry it came from to see if there are any good suggestions in the comments. The only thing there is one "as long as it's opt-in" and two wanting to redesign the entire inbox/messaging structure entirely, so figuring out how to implement this is up to me.
2. I take a look at the compose page. My first thought is that the best way to implement this would be to have a "CC myself" checkbox, that if checked would make a copy of the notice go to the person who sent it. (It could probably even fit nicely above the subject line, right beneath the "To:" field on a separate line.)
3. My next step is to go look at the code for that page and see if I can figure out how I might do it. A quick glance over things tells me that it should be relatively easy: make the checkbox as part of the form that's generated, give the checkbox a value, and if the value is true on submit, do something in lines 63-69 to put the person sending the message into the @to_list variable.
4. Right about now is when I start to think: okay, but what if people will always want to keep a copy of their sent messages in email? It might get annoying if they always have to remember to check the box. We could make the box always checked, and let people who don't want copies of their messages uncheck it, too, but that's got the same problem.
I know from previous bugs that it would be relatively trivial to create a userprop -- "opt_always_cc_mesages" or something like that -- and a new setting in the Account Settings page to govern the setting of it. I also know that it would be pretty trivial to poll that userprop at the time of rendering the PM interface to determine whether or not the checkbox should be checked by default.
But I also know that adding more options like that is not always a good thing. General standards of usability design say that adding more options should be something you do as little as possible: the goal is to pick useful defaults, and so I sit and think about this for a little bit. My personal standard for creating something as an option is, more or less, "do I think more than about 30% of our users are going to want to fiddle around with whatever choice we make for them" with a side helping of "if less than 30% of our users are going to want to fiddle with it, how desperately are they going to want to fiddle with it", and I don't think this is major enough to qualify.
Eventually I decide that the best thing to do is to keep it as an at-the-time-of-composing option, rather than a sitewide option. That narrows the question down to: should the checkbox default to checked or unchecked? Deciding that is going to come down to whether I think that more people are going to want to send themselves CCs of messages they send or not send them, so I poll irc to see what people think. I get a few answers that mostly trend towards "unchecked", and after I go back to the suggestions poll and see how the poll shook out, there are enough people voting "no" or "with changes" on the suggestion that I don't think "checked" should be the right default. (If I'm wrong, I'm certain there'll be a suggestion to change it coming along shortly after the code gets pushed, and I can see how the discussion turns out in that suggestion.)
5. So, that decided, it's time to get down to implementation. I start by opening up inbox/compose.bml and inbox/compose.bml.text. With my workflow, I always like to add the text strings first -- that way I don't forget them -- so I turn to inbox/compose.bml.text and get ready to add a label for the textbox... except the only thing in that file is an error message for what happens when the recipient is suspended, which means that the text for building the form lies elsewhere.
(The inbox and associated text strings are complicated, I've realized.)
So, it's back to the compose page on DW itself, with ?uselang=debug:
http://www.dreamwidth.org/inbox/compose.bml?uselang=debug
...only to discover that most of the compose page isn't stripped to begin with. That distracts me temporarily into going over to Bugzilla and seeing if there's a bug open to English-strip that page. I search on both "English-strip", "strip compose", and "strip inbox" and don't find anything, so I pause and submit Bug 1528 to do that in the future. I won't do the stripping as part of this bug -- it's bad form to combine functional changes like that -- but I will make sure that my changes to the form will be stripped.
So, I add in the text that I'm going to add as
.form.label.cc and give it the text of "Send me a copy of my own message". I can safely close that file now, since I'm pretty sure that's the only thing I'm going to change or need, text-wise. I make a note of what I called the string and go back to the other file.6. My particular workflow tends to be "change the page that displays first, then change the did_post section". (Most pages are combined "what to show someone who first visits the page" and "what to do, and to show someone, after a form has been submitted"; you can find the "after a form has been submitted" in the block that follows an
if (LJ::did_post()) { beginning if statement, and the "first visit" after the subsequent } else { of that block.) I do it that way so I can verify that the form is being changed successfully before I try to change what happens when you submit it -- if you do it the other way around, you have to code the whole thing before you can test it, and if you screwed something up and it's working wrong, you have a harder time figuring out what part you screwed up.
So, I go down to that section. I want to stick it before the Userpic selection -- at least, I think I do -- so I stick it right after line 231:
# Are we sending a copy of the message to the user?
if ( $remote ) {
$body .= LJ::html_check( {
name => 'cc_msg'
id => 'cc_msg'
value => 1,
selected => 0,
} );
$body .= " <label for='cc_msg'>" . BML::ml( '.form.label.cc' ) . "</label>";
}(Notice that I comment my code! It's always a good idea to comment whenever you're adding a new block of logic or whatever, just so that the next person who comes along will know what you did and why you did it.)
I'll also confess that I always forget how to build those checkboxes. Always. A lot of what I do while coding is think "okay, where else in the code is there something else that does something like what this is doing here", and then go look and see how they did it. Since I can never remember the various HTML controls, and they're poorly documented in-code in htmlcontrols.pl, I always wind up looking around via grep.
7. Save and view the page on my hack. Uh-oh:
[Error: syntax error at '/dreamhack/home/8083-rahaeli/dw/htdocs/inbox/compose.bml' line 236, near "'cc_msg' id" syntax error at '/dreamhack/home/8083-rahaeli/dw/htdocs/inbox/compose.bml' line 278, near "} }" @ hack.dreamwidth.net]Looks like I forgot some commas. It should be:
# Are we sending a copy of the message to the user?
if ( $remote ) {
$body .= LJ::html_check( {
name => 'cc_msg',
id => 'cc_msg',
value => 1,
selected => 0,
} );
$body .= " <label for='cc_msg'>" . BML::ml( '.form.label.cc' ) . "</label>";
}(This is one of the most common mistakes I make, so I figured I'd share it to point out what are good things to look for when you get errors. It's a good idea to start keeping lists of your own most common mistakes, so you can look over what you're coding for those mistakes even before you check them.)
8. Save and reload. The checkbox displays properly now, but the styling on it is weird to my eyes: bold, like the rest of the header items, and the padding on the bottom is a little weird:
It's functional, it's just ugly there. It breaks up the evenness of having the bolded labels on the left and the action items on the right. So, I decide I want to move it down to beneath the Userpic line. I pick up my code and move it down below the line that looks like this:
# The drop-down userpic menu
$body .= $picform . "</div>";Reload and .... oh dear:
Looks like having this checkbox outside the closing <div> after $picform is pushing it out to the side. I don't like that, so we're going to have to rearrange things some more.
I can't just take the out of the drop-down userpic line and put it onto the end of the checkbox lines, because the checkbox lines only get called if there's a remote user. But ... that actually makes me wonder; is it even possible to have a situation where there's no remote user viewing the page? Is it possible to message someone while you're logged out? I check the top of the file, and sure enough, there has to be a $remote (remote user), or else the viewer of the page gets redirected to log in.
That means I can safely dispose of the if-block surrounding the section I'm adding, and move the closing of the <div> to that block:
# The drop-down userpic menu
$body .= $picform;
# Are we sending a copy of the message to the user?
$body .= LJ::html_check( {
name => 'cc_msg',
id => 'cc_msg',
value => 1,
selected => 0,
} );
$body .= " <label for='cc_msg'>" . BML::ml( '.form.label.cc' ) . "</label></div>";Save and reload, and I get:
I almost like that, but I still don't want it bolded, or quite as big. I change the last line to:
$body .= " <label for='cc_msg' style='font-weight: normal; font-size: smaller; font-style:italic;'>" . BML::ml( '.form.label.cc' ) . "</label></div>";";
Reload, and I like it now:
9. So, all the styling aside, now it's time to go back and make it so that the box does something when someone hits submit. I go back to the section of code in lines 63-69 that govern recipients, and take another look at it.
I'm pretty sure that all I have to do here is push $remote onto @to_list if the box is checked, like thus:
my @to_list = keys %to_hash;
push @to_list, $remote if $POST{'cc_msg'};Save, and now it's time to go test.
10. In order to test private messaging, you have to be running TheSchwartz, and specifically running the worker that deals with messaging. There's a wiki page about getting TheSchwartz set up, but it doesn't quite cover getting it set up on Dreamhacks properly -- I had Sophie's help in getting mine set up, but I can't remember what I did now, so I make a mental note to bug someone to put better instructions in the Wiki for how to set up TheSchwartz on Dreamhacks.
I'm lucky enough to have it already set up, though, so I can just start it up:
$LJHOME/bin/worker-manager --debugI put in a message, hit "send", and get:
There was an error processing your request:
"LJ::User=HASH(0x6d6ddd0)" is not a valid username
Oops.
11. Back to the code. The problem is almost certainly that I didn't put $remote into the right form on that line I added. (I still don't understand the exact this-and-that about what a user object actually is.) I'm going to start off by guessing that $remote is already in whatever form LJ::load_user returns, so when you run it twice -- in the subsequent foreach loop -- it screws something up.
So, I look at the lines before, and see that the other items in the @to_list are usernames, not user objects. Let's try putting the $remote variable into the same form:
push @to_list, $remote->username if $POST{'cc_msg'};Save and try again.
12. This time, the message went through without any errors, which is a plus. I check my inbox, and ... there it is! No email though ... oh, I have the "by email" option checked off for that subscription in my settings. I check it, send the message again, and success! I have a copy of the message in my inbox and in my email.
I also run a negative test: send a message without the box being checked, and make sure I don't get a copy of the message. I don't, so we're good there.
13. ...Except I just realized, doing it this way makes it appear as though you've sent the message to yourself, not to whoever you sent it. (This is because the multiple-message model kinda stinks.)
Now I have a dilemma. Do I try to fix that by pulling out a separate routine to send the email to yourself, with a list of recipients inside the actual email, or do I say that this is good enough and let it go? Advantage to letting it go: I'd be done with the bug. Also, there'd be less code reuse, because the only way I can think to make this work is going to be either to do a lot of refactoring, or to put in another message-sending section, and I'm pretty sure that if I do that, I'm going to have to add another translation string.
The advantages to fixing it, though, outweigh the other advantages: if I leave it like it is, every time I look at it, I'll know that I half-assed this. Dammit. Stupid work ethic.
14. Okay. I go back to the file, and the actual sending of the message is in line 131. The problem is, it's using another function -- LJ::Message->send -- that may actually not be able to do what I want it to do. Let me go and see what it looks like... A structure like that means that the function probably lives in cgi-bin/LJ/, and sure enough, it's in cgi-bin/LJ/Message.pm.
Except I take a look at it, and it's pretty much gibberish to me -- way, way beyond my ability to read, much less hack.
I've got two options here: find someone who does understand the event-sending code to give me a hand, or submit my patch as-is and open up a new bug for someone to fix it up later. At this point, I'm getting tired of working on this bug (and really tired of doing this writeup), and we're having a party tomorrow that I should be helping prep for, so I invoke my fifteen-minute rule: if I can't figure out how to do something in fifteen minutes or less, I pass on it and move along.
So, once this patch gets added, I'm going to open a new bug to fix it. I call this one done, and generate and submit the patch.