CommitReviews
Contents
- 1 e3455e7285f63e340c4147682df59d4683a25d96
- 2 26cdc69c0335f6b4c3d77cafb6ffd41c245fce35
- 3 77a3a6c3741c0f31a588fba4025c302e1c7b21e4
- 4 c143aac328a04aac1722d7d8450425d94a7f5a34
- 5 0360ae9a7d5249b0247e21c43a62d31823d9cee7
- 6 2656afa0b56b7d6dbe9a3e46a1dd7f34c6945b6f
- 7 65b6e1ef55154079b6a4c10d2dca6edc515a447f
- 8 53326946705a525e14d285ab9258f0cd3b06d3b6
- 9 5d31b9f1a8050bf83e41e1383e4564b716c8e058
- 10 3dba97991d431e0a837a2c7bb087c6c092d232f9
- 11 cc64b4a0613573699d8a07ca1843ecfe0581e5be
- 12 a2be3aaeb26f74bfc33017297c6b5ad3ec199921
- 13 44136fbcccd24dec43d8699a7d6fd893eda47085
- 14 4fe1cb2a4f534bc3f608cf780120100347cfbb5d
- 15 76d8f9d36f588e6463714bbee9dcac14337e60e7
- 16 564b73d0b639dfc404c4012bc99b8439e52f0975
- 17 8cf78ea5c26f11cbd7442569f092e8e75eea62ed
- 18 f5cb69d2919478cddfaf301d540b9305c9ca4c78
- 19 8dafe6989547f9b595006872e65cc3417d96211c
- 20 d1956e6e8bdfa76abfdad6e379dac99950ee5734
- 21 11140606bceb4fcde379b49f3fed720ee02eaa86
- 22 c2e269909db1f5029d0b464f70bee812435c7a0d
- 23 032c842930cce532d601d971e4c3a9ce407535ee
- 24 76269b0efc9995bdf3c39a7b6f5430fafce909a3
- 25 ed5771caef623861ce9874efe6966de924f38ffe
e3455e7285f63e340c4147682df59d4683a25d96
commit e3455e7285f63e340c4147682df59d4683a25d96 fixed bizarre template bug. the test only failed when ran as part of the entire test suite. everything seemed to work correctly, but the instance variable ...
Content: I see that the line:
user = session[:user] ? session[:user] : User.new
has been removed, but I can't figure out where is the user being initialized. The functionality seems to be working fine, but how?
26cdc69c0335f6b4c3d77cafb6ffd41c245fce35
commit 26cdc69c0335f6b4c3d77cafb6ffd41c245fce35 Heavy jobs: changed test in an attempt to explain errors on production
Style: One extra whitespace line
Content: Perhaps it is a good test case, but it does not convey what it tests. I might be missing something, so probably some background on which error it was trying to explain could have made into the comment.
77a3a6c3741c0f31a588fba4025c302e1c7b21e4
commit 77a3a6c3741c0f31a588fba4025c302e1c7b21e4 Actions: added a new feature--NOT selectors, did some refactoring
Style: Trailing white space yuck. When you use git diff and git status, do you have color highlighting? If not, please enable it. Here is how:
git config --global color.diff auto git config --global color.status auto git config --global color.branch auto
Once you enable the colors you'll see the yuck you are creating and be able to easily fix it before committing it.
Content: There should have been two commits, one for the new selector and the other for the routes. But the overall functionality that the commit provides is good. Although, its hard to think that where this new selector will be used.
Interesting test case for the new entry in routes. When you first see it, looks like that the test does nothing, but it would actually gives an error if that route is not there. But again, this does completely test the route to verify it goes to the right controller and action. what's the best methodology for testing the routes file?
- Use assert_generates and assert_recognizes to test routes. We haven't really been testing routes much because the higher level tests catch so many of the routing issues. I don't remember ever being bitten by routing issues on production. Especially in code that has good higher level test coverage. Not sure it's worth it to explicitly test routes? --Brandon CS Sanders
c143aac328a04aac1722d7d8450425d94a7f5a34
commit c143aac328a04aac1722d7d8450425d94a7f5a34 Actions: test now doesn't fail non-deterministically; fixtures work properly
Style: Minor white space issue.
Content: Nice little commit. I was wondering why do we have to do Model.delete_all as rails cleans the database and loads fixtures at the start of every test case?
- Rails only cleans out the tables specified in the fixtures directive and not all of the tables we are using have been specified. I haven't enjoyed my experiences with fixtures so far, so haven't been using them for new code that I write. Maybe need to try to reintroduce fixtures? My biggest issue is the dependencies they introduce to tests are spread out across the fixture files. For example, you end up with huge fixture files with little parts being used by many different tests. If you want to understand the tests, you have to go pull out the right pieces of a bunch of different fixture files. If you want to understand the fixtures (to clean them up for example), you have to go read a whole bunch of test files.
0360ae9a7d5249b0247e21c43a62d31823d9cee7
commit 0360ae9a7d5249b0247e21c43a62d31823d9cee7 ActionsUI:taking care of crashes in nil cases
Style: is fine.
Content: A good fix to the actions framework. Making sure that the partial view(_actions) for actions does not crash in case the variable actions is nil. And a good test in the end to confirm the correct functionality.
2656afa0b56b7d6dbe9a3e46a1dd7f34c6945b6f
commit 2656afa0b56b7d6dbe9a3e46a1dd7f34c6945b6f Enhanced AR#find: PageCache uses a seperate database in production. Hence, ugly hack needed to override default class method :(
Style: Beautiful
Content: A nice hack (to support his previous beautiful hack :-) ) keeping in mind that the database used for caching on production is different from the local installments.
Suggestion: This hack wouldn't be required if our local db setup replicates the production environment. Interesting idea ... I'd like to move in this direction. Shouldn't be hard to modify our setup and the au-tools repo.
65b6e1ef55154079b6a4c10d2dca6edc515a447f
commit 65b6e1ef55154079b6a4c10d2dca6edc515a447f Actions: small refactoring
Style: stylish
Content: uses ruby's built-in iterators, making the code more readable. This commit is itself the best kind of code-review. It notices a bit of code that goes against the grain of ruby and creates an efficiently expressed suggestion about how to modify to be more in harmony with ruby. This kind of commit is probably the best way to improve our collective style ... so long as we notice them
53326946705a525e14d285ab9258f0cd3b06d3b6
commit 53326946705a525e14d285ab9258f0cd3b06d3b6 Enhanced ActiveRecord#find: Monkey-patched activerecord's find method to take an extra option that tells it to run the find query on master if set. This patch should work for ActiveRecord's normal finds and dynamic finders. However, find_by_sql is not yet covered. This should hopefully allow us to avoid duplicate record creations because of database replication lag.
Style: Excellent
Content: Excellent fix to a long-standing problem! Every time it cropped up, we thought long and hard how to resolve it, and then after considering it for a while, opened a connection to master db and did the work, solving the same problem multiple times. This takes care of the issue once and for all!
But probably we need to use the built-in validate_options function after first rejecting the 'use' parameter, and may be, use 'active_record_extended' instead of 'extended_active_record' as the name of the file, to make it locate faster.
- Regarding using built-in validate_find_options, it was in my to do list. This has been incorporated. Thanks. --Ali Aslam 05:17, 30 May 2008 (PDT)
- This is exactly the sort of review and response I hoped for from our CommitReviews. I'm going to stop pointing out that you guys are doing a great job reviewing so that I don't noise up the signal. But know that I am noticing and appreciating!
5d31b9f1a8050bf83e41e1383e4564b716c8e058
commit 5d31b9f1a8050bf83e41e1383e4564b716c8e058 resolved race condition issue with page cache by using optimistic locking
Style: Looks Good.
Content: Great Fix to an issue that was flooding the error logs. The test case is really nifty; especially liked the clever use of flexmock to mock a page_cache race condition. Good Work! Yay!
Additional Information: I was just wondering with the current code, can a situation arise where recursion goes 2 steps down. Off the top of my head, cannot see this happening; nevertheless, better to be safe than sorry.
- Retracting my assertion about recursion not going beyond 2 levels. Since find is executed on a slave, any number of recursions are possible until db replication has kicked in. --Ali Aslam 03:29, 30 May 2008 (PDT)
Possible Improvement could be to use find from master db when recursion = 1. This is since we know master has the record created. Going to incorporate it.
3dba97991d431e0a837a2c7bb087c6c092d232f9
commit 3dba97991d431e0a837a2c7bb087c6c092d232f9 CleanErrorLog: Make sponsor/portal urls innaccessible The action no longer existed in the controller, but the template was being rendered when the action was being called. Someone probably has a bookmark to it, or it's somewhere in Google search results.
Style: Not Applicable.
Content: Good Fix to a rails created nuisance.
Additional Information: Rails tries to perform requested action by first invoking that action method followed by render. However, the nuisance part arises for the case where the action method does not exist. In this case, it tries to locate a template file with the same name as action and renders it if it's not a partial template. Failing both, it raises the 'no action' exception. Hence, another way of bypassing this nuisance is to make your orphan templates partial. Doing so has the side effect of not breaking other controller#action methods that were rendering this template. Good idea and suggestion, but I don't think the last sentence is quite right. Partials are treated differently from normal templates. If you add a leading underscore, it won't be found any more than a template with a completely different name would be. I think it is better to make it clear that it isn't currently being used than to leave it in a state that makes it look like it might be being used.
cc64b4a0613573699d8a07ca1843ecfe0581e5be
commit cc64b4a0613573699d8a07ca1843ecfe0581e5be Fix error message to not crash. Still need to clean up the database so this error doesn't exist any longer.
Style: No problems.
Content: This syntax error would have been caught if we had a test checking this behavior. This just shows the worth of having tests with good coverage as advocated by Brandon et al. Otherwise, to err is human :)
Additional Information: As regards cleaning up sources table for duplicate anonymous users, thunderclap today did not show any duplicates. I think this issue of Multiple Anonymous Users is behind us.
a2be3aaeb26f74bfc33017297c6b5ad3ec199921
commit a2be3aaeb26f74bfc33017297c6b5ad3ec199921 fixed sporadically failing test due to edit time being out of sync with page being edited
Style: 3 Whitespace yucks. Good fix to include parentheses in method definition ... should always have parens in definition, even if they are often left out of calls.
Content: Well-encapsulated fix to a nuisance. Need to notice whether there are other places with similar race conditions.
44136fbcccd24dec43d8699a7d6fd893eda47085
commit 44136fbcccd24dec43d8699a7d6fd893eda47085 Edit: page_is_redirect and redirect table are now updated correctly
Style: 8 Whitespace yucks. Clear, concise comment.
Content: I didn't realize that you don't need a space between #REDIRECT and the link to redirect to. Good test ... makes it easy to understand your fix and trust that it works.
4fe1cb2a4f534bc3f608cf780120100347cfbb5d
commit 4fe1cb2a4f534bc3f608cf780120100347cfbb5d Edit: Dont build domain article for pages without default namespaces + Added Test
Style: 1 Whitespace yuck.
Content: Good clean commit ... good fix.
76d8f9d36f588e6463714bbee9dcac14337e60e7
commit 76d8f9d36f588e6463714bbee9dcac14337e60e7 PageView: Show table of contents if showtoc in user_options is set + Added Tests
Style: 4 Whitespace yucks. (These are lines that end in whitespace. Fix your git diff to show colored diffs and you'll notice when you are adding them.)
Content: Good fix. Now we'll never regress on this again.
564b73d0b639dfc404c4012bc99b8439e52f0975
commit 564b73d0b639dfc404c4012bc99b8439e52f0975 ActionsPatrolling: Displayed diff in details for PageRevise and PageCreate actions
Nice to have so much beauty show up in a single small commit. Interesting that we don't really have much testing associated with views. I wonder whether an integration test here would make sense? Or if it would just add brittleness to our test suite and slow it down without adding much value?
8cf78ea5c26f11cbd7442569f092e8e75eea62ed
commit 8cf78ea5c26f11cbd7442569f092e8e75eea62ed Action: Marked all catch actions as patrolled
No tests with this one? I think it should actually mark it as unpatrolled if there is an associated unpatrolled RC, otherwise mark it patrolled.
f5cb69d2919478cddfaf301d540b9305c9ca4c78
commit f5cb69d2919478cddfaf301d540b9305c9ca4c78 Actions: Updated catchup actions to work in all cases
Good commit: Does one thing, includes a solid test of that one thing. BUT, adds trailing whitespace (yuck). Use the triple dot range operator (...) rather than the double dot to create an exclusive rather than inclusive range. Then you don't need the -1 in 1...revisions.length. Good commit.
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.
- More in well-tested ruby seems like the right direction to me. --Brandon CS Sanders
- MediaWiki Parser's preSaveTransform does the following things:
- Replaces \r\n with \n
- Converts the 4 tildas with user's signature (user IP in case of anonymous user).
- Renders image Gallery (if in wiki text)
- Handles nowiki, html and some other tags
- etc.
There's a lot of missing functionality which we aren't addressing, yet a good start with rails version of Parser's functionality. --Ali Anwar 05:46, 28 May 2008 (PDT)
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.