Tag Archives: refactoring

Groundwork for new features in OpenStreetMap

Last summer I finished a large refactoring, and thought it would be nice to change tack, and so I decided to try to push through a new feature as my next project. Refactoring is worthwhile, but it has a long-term pay-off. On the other hand, new features show progress to a wider audience, and so new features are another avenue to getting people interested and involved in development.

I picked an old Google Summer of Code project that hadn’t really been wrapped up, and immediately spotted a bunch of changes that would be needed to make it easier for others to help review it. Long story short, it needed a lot more work than anyone realised and it’s taken me a few months to get it ready. I’ve learned a few lessons about GSoC projects along the way, but that’s a story for another time.

I want to keep going with the refactoring, since a better codebase leads to happier developers and eventually to better features. But it’s worthwhile having some sort of a goal, otherwise it’s hard to decide what’s important to refactor, and to avoid getting lost in the weeds. There have been discussions in the past about adding some form of Groups to OpenStreetMap, and it’s a topic that keeps on coming up. But I know that if anyone tried implementing Groups on top of our current codebase, it would be impossible to maintain, and it’s far too big a challenge for a self-contained project like GSoC.

So what things do I think would make it easier to implement Groups? The most obvious piece of groundwork is a proper authorisation framework. Without that, deciding who can view messages in or add members to each group would be gnarly. I also don’t want to add many more new features to the site with our “default allow” permissions - it’s too easy to get that wrong, particularly adding something substantial and complex like Groups.

I had a stab at adding the authorisation framework a few weeks ago, but quickly realised some more groundwork would help. We can make life easier for defining the permissions if we use standard Rails resource routing in more places. However, that involves refactoring controller methods and renaming various files. That refactoring becomes easier if we use standard Rails link helpers, and if we use shortened internationalization lookups.

So there’s some more groundwork to do before the groundwork before the groundwork…

This post was posted on 11 April 2018 and tagged development, OpenStreetMap, rails, refactoring

Factory Refactoring - Done!

After over 50 Pull Requests spread over the last 9 months, I’ve finally finished refactoring the openstreetmap-website test suite to use factories instead of fixtures. Time for a celebration!

As I’ve discussed in previous posts, the openstreetmap-website codebase powers the main OpenStreetMap website and the map editing API. The test suite has traditionally only used fixtures, where all test data was preloaded into the database and the same data used for every test. One drawback of this approach is that any change to the fixtures can have knock-on effects on other tests. For example, adding another diary entry to the fixtures could break a different test which expects a particular number of diary entries to be found by a search query. There are also more subtle problems, including the lack of clear intent in the tests. When you read a test that asserts that a given Node or Way is found, it was often not clear which attributes of the fixture were important - perhaps that Node belonged to a particular Changeset, or had a particular timestamp, or was in a particular location, or a mixture of other attributes. Figuring out these hidden intents for each test was often a major source of the refactoring effort - and there’s 1080 tests in the suite, with more than 325,000 total assertions!

Changing to factories has made the tests independent of each other. Now every test starts with a blank database, and only the objects that are needed are created, using the factories. This means that tests can more easily create a particular database record for the test at hand, without interfering with other tests. The intent of the test is often clearer too, since creating objects from factories happens in the test itself and are therefore explicit about what attributes are the important ones.

An example of the benefits of factories was when I fixed a bug around encoding of diary entry titles in our RSS feeds. I easily created a diary entry with a specific and unusual title, without interfering with any other tests or having to create yet another fixture.

def test_rss_character_escaping
  create(:diary_entry, :title => "<script>")
  get :rss, :format =&gt; :rss

  assert_match "<title>&lt;script&gt;</title>", response.body
end

All in all, this took much, much longer than I was expecting! Looking back, I feel I might have picked a different task, but I’m glad that it’s all done now. I’m certainly glad that I won’t have to do it all again! Moving on, it’s now easier for me and the other developers to write robust tests, and this will help us implement new features more quickly.

Big thanks go to Tom Hughes for reviewing all 51 individual pull requests! Thanks also to everyone who lent me their encouragement.

So the big question is - what will I work on next?

This post was posted on 1 June 2017 and tagged development, OpenStreetMap, rails, refactoring, tests

Steady progress on the OpenStreetMap Website

Time for a short status update on my work on the openstreetmap-website codebase. It’s been a few months since I started refactoring the tests and the work rumbles on. A few of my recent coding opportunities have been taken up with other projects, including the blogs aggregator, the 2017 budget for the OSMF Operations Working Group (OWG), and the new OWG website.

With the fixtures refactoring I’ve already tackled the low-hanging fruit. So now I’m forced to tackle the big one - converting the Users fixtures. The User model is unsurprisingly used in most tests for the website, so the conversion is quite time-consuming and I’ve had to break this down into multiple stages. However, when this bit of the work is complete most future Pull Requests on other topics can be submitted without having to use any fixtures at all. The nodes/ways/relations tests will then be the main thing remaining for conversion, but since the code that deals with those changes infrequently, it’s best to work on the User factories first.

As I’ve been working on replacing the fixtures, I’ve come across a bunch of other things I want to change. But before tackling all that I’m going to mix it around a bit. My goal is to alternate between the work I think is the most important, and also helping other developers with their own work. We have around 40 outstanding pull requests and some need a hand to complete. There are plenty of straightforward coding fixes among the 250 open issues that I can work on too. I hope that if more of the issues and particularly the pull requests are completed, this will motivate some more people to get involved in development.

If you have any thoughts on what I should be prioritising - particularly if you’ve got an outstanding pull request of your own - then let me know in the comments!

This post was posted on 21 February 2017 and tagged development, OpenStreetMap, rails, refactoring

Refreshing the OpenStreetMap Codebase

The codebase that powers OpenStreetMap is older than any other Rails project that I work on. The first commit was in July 2006, and even then, that was just a port of an existing pre-rails system (hence why you might still see it referred to as "The Rails Port")

It's a solid, well-tested and battle-hardened codebase. It's frequently updated too, particularly its dependencies. But if you know where to look, you can see its age. We have very few enduring contributors, with is surprising given its key position within the larger OpenStreetMap development community. So I've been taking a look to learn what I can do to help.



For someone just getting started with Rails, they'll find that many parts of our code don't match what's in any of the books they read, or any the guides online. More experienced developers will spot a lot of things that were written years ago, and would nowadays been done differently. And for some of our developers, particularly our Summer of Code students, they are learning what to do by reading our existing code, so our idiosyncrasies accumulate.

I started trying to fix a minor bug with the diary entries and a bunch of things struck me as needing a thorough refresh. I started the process of refactoring the tests to use factories instead of fixtures - 2 down, 37 to go. I've started rewriting the controllers to use the standard rails CRUD method names. And I've made a list of plenty of other things that I'd like to tackle, all of which will help lower the barrier for new (and experienced) developers who want to get stuck into the openstreetmap-website code.

I hope that progress will snowball - as it becomes easier to contribute, more people will join in, and in turn help make it even easier to contribute.

But it's time-consuming. I need help to share these projects around. If you're interested, please get stuck in!

This post was posted on 15 September 2016 and tagged development, OpenStreetMap, rails, refactoring

subscribe via RSS