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?

modules.zendframework.com/module/Application/test/ApplicationTest/Service/RepositoryRetrieverTest.php

Line 201 in 8af567e

public function testRepositoryFileContentFails()

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]

camelCase? Also, above.

[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]

You should still set a 503 response code, even if the page is correctly generated.

[adamlundrigan]

Probably a good idea, but outside the scope of this PR. The github sync process needs to be rewritten (#349) and @ins0 is working on the user panel stuff in #351

[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 👍) but the principle behind the change is the same: catch any exception coming from EdpGithub and dump out an error message to the user.