Difference between revisions of "CommitReviews"

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.

-Stephen Judkins

• 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.