Display GitHub error message when module sync fails

[adamlundrigan]

Catch the RuntimeException thrown by EdpGithub and send it back to the user.

Two notes:

• I tried to test it but failed. I couldn't find the right incantation to get PHPUnit's mock builder to build a suitable replacement for EdpGithub's RepositoryCollection. Perhaps someone with a kinder disposition towards mock builder could try?
• I looked through EdpGithub and GitHub's docs and I don't see any case where sensitive information could be leaked through the failure message, so IMO it should be fine to just dump it back to the user for now.

[ins0]

maybe we move the error catch also to the RepositoryRetreiver class? https://github.com/zendframework/modules.zendframework.com/blob/8af567eaa08f2f67ecf6c0f55998065201ff0cf0/module/Application/test/ApplicationTest/Service/RepositoryRetrieverTest.php#L201 mocked a error case with the github client - helpful?

[adamlundrigan]

It's a largely cosmetic change anyway so I didn't put too much effort into writing a test for it...that whole controller is untested and needs a refactor to make that feasible. You're right though, if I mock the client instead of the collection I'd likely have an easier time.

[localheinz]

@adamlundrigan

Not sure about passing through the actual exception message, as you pointed it out, but looks good to me!

[adamlundrigan]

@localheinz thanks for the reivew. I've fixed the items you pointed out.

As for returning the exception message it's not ideal but it's a good stop-gap to keep the spinner from spinning forever if there is a problem with the sync. Ideally EdpGithub or the RepositoryRetriever service would throw distinguishable exceptions (or at least exception messages we know to be safe to send back to the user)

[Ocramius]

I'd still ask for a unit test of this functionality (mock with ->throwsException())

[adamlundrigan]

@Ocramius I mocked all the things; there are now four test cases covering the changes made in this PR

[localheinz]

@adamlundrigan

Needs a rebase!

[Ocramius]

This needs another rebase, sorry for letting it slip through, @adamlundrigan :(

[GeeH]

Can this be merged

[GeeH]

Ping @Ocramius @adamlundrigan

[Ocramius]

It needs a rebase before it can be merged

[adamlundrigan]

                                                                                  I'll take a look today

[adamlundrigan]

The code had changed so much I ditched my original and implemented it fresh, integrating your suggestions:

• RepositoryRetriever now catches the exception and returns false (@ins0), which is more consistent with the way other methods in RepositoryRetriever behave.
• indexAction and listAction return 503 when an error is detected (@ocramius)
• The actual exception message is no longer passed through to the user (@localheinz)
• Swapped out $.load for $.ajax in the views as the former only handles success conditions.

I was no longer able to reproduce the original API limit exceptions against GitHub (kudos to whomever fixed the sync :+1:) but the principle behind the change is the same: catch any exception coming from EdpGithub and dump out an error message to the user.