CommitReviews
Contents
- 1 8dafe6989547f9b595006872e65cc3417d96211c
- 2 d1956e6e8bdfa76abfdad6e379dac99950ee5734
- 3 11140606bceb4fcde379b49f3fed720ee02eaa86
- 4 c2e269909db1f5029d0b464f70bee812435c7a0d
- 5 032c842930cce532d601d971e4c3a9ce407535ee
- 6 76269b0efc9995bdf3c39a7b6f5430fafce909a3
- 7 ed5771caef623861ce9874efe6966de924f38ffe
8dafe6989547f9b595006872e65cc3417d96211c
commit 8dafe6989547f9b595006872e65cc3417d96211c
Author: Stephen Judkins <<email>5b125b163d077172d60fe6b72b4ef2ae</email>>
Date: Tue May 27 12:18:37 2008 -0700
pre-save transform now in pure ruby; functional tests execute about 2x as quickly
I was getting frustrated by the speed of our functional test suite, so I replaced the PHP pre-save transform with a pure-Ruby version. We are calling pre-save transform again anyways, but this change means we only have to invoke PHP once. If this introduces bugs, please feel free to revert the code to the way it was. All I ask is that you write tests that exercise this bug--that is, they work with the code the old way and break with this change. Then--unless anyone has any objections, which I will hear out--I plan to change the MediawikiParser.cleanup method to make the new tests pass.
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)
- In Mediawiki, the offset is used to display times in the local timezone of the user. I'm not sure whether we are doing this yet with compost. It is a nice feature that I think we'll probably implement sooner rather than later. --Brandon CS Sanders
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?
- The commit did the following:
- Moved checkboxes + textboxes in a separate array.
- Removed an unused if-clause (with already no tests) + Indentation
- Parenthesized arguments of email_invalid?
- Fixed reset button's type to reset.
So I guess the old tests serve their purpose for the refactor. The removing of newline from description is however in commit 11140606bceb4fcde379b49f3fed720ee02eaa86 which was just presentation. --Ali Anwar 22:26, 25 May 2008 (PDT)
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.
- Tests written. Thanks for pointing out. --Ali Anwar 23:14, 25 May 2008 (PDT)
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.