Difference between revisions of "CommitReviews"

d1956e6e8bdfa76abfdad6e379dac99950ee5734

commit d1956e6e8bdfa76abfdad6e379dac99950ee5734

    Watchlist Notifications: Suppressed email delivery errors to guard against
    action mailer raising smtp failure errors on failed email delivery.
    
    In such a scenario, the user is shown owls whereas only an email failed to deliver.
    This shouldn't be part of the loop.
    One reason for this failed delivery is bad email addresses which our current codebase
    doesn't catch.

Seems like an appropriate fix. AND, recently we had an error where our smtp server was down and users reporting the owls helped us find and fix it. After making this change an email to addressed specifically to Ethan would be good, asking whether or not we have monitoring in place for our smtp server. Doing this now.

11140606bceb4fcde379b49f3fed720ee02eaa86

commit 11140606bceb4fcde379b49f3fed720ee02eaa86

    UserPreferences: Removed unused control from view + Fixed typo

Why is this control unused? Guessing the offset is very helpful.

• I gave our compost + mediawiki code a little look and didn't quite find any use of the time offset. Its in my TODO list to dig further and understand its functionality better, which is why I haven't yet removed it from the user controller. Thanks for pointing this out. --Ali Anwar 22:17, 25 May 2008 (PDT)

c2e269909db1f5029d0b464f70bee812435c7a0d

commit c2e269909db1f5029d0b464f70bee812435c7a0d

    UserPreferences: Huge refactor + Fixed minor issues

Big commit without changing tests ... seems okay though. Refactorings shouldn't add functionality, so the existing tests are enough. This one also fixes a minor issue (removes a newline from a description). I guess this is presentation only?

032c842930cce532d601d971e4c3a9ce407535ee

commit 032c842930cce532d601d971e4c3a9ce407535ee

    Preferences: Removed html tags from email views to stop them rendering in the emails being sent.

It would be nice to know how to send an html email and have the option of sending it, or the plain text.

76269b0efc9995bdf3c39a7b6f5430fafce909a3

commit 76269b0efc9995bdf3c39a7b6f5430fafce909a3

    User: Fixed user_options update bug

No Test: Fixing a bug should almost always come with a test that exercises that bug. How do you know that you've fixed the bug? How do you know that it won't come back? Now I have to read and understand your code to figure out what the bug is rather than having a clear test that shows me what it is.

ed5771caef623861ce9874efe6966de924f38ffe

commit ed5771caef623861ce9874efe6966de924f38ffe

    Preferences: Removed an assert which seemed a bit unlogical

You've found a test that doesn't communicate it's intention very well. Instead of removing a part that seems illogical, you should probably find out what the test is getting at and improve it to read better. In doing this I discovered that you can save empty passwords, which seems to violate the intention of the original test. I fixed with this commit 09ef499534429c7567ffa46cc5a83257e0d447f7

Lesson: convoluted tests are worse than convoluted code ... make sure that your tests are easy to read and clearly represent the design of your functionality.