Code review comment for lp:~mvo/software-center/polkit-lp785117

On Wed, Aug 22, 2012 at 8:57 AM, Michael Vogt <email address hidden> wrote:
> === modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
> --- softwarecenter/backend/installbackend_impl/aptd.py 2012-08-07 13:15:08 +0000
> +++ softwarecenter/backend/installbackend_impl/aptd.py 2012-08-22 07:56:18 +0000
> @@ -973,13 +979,48 @@
> + return
> + if name in ["org.freedesktop.PolicyKit.Error.Failed"]:
> + summary = _("Authentication Error")
> + text = _("Is policykit installed and an authentication "
> + "agent running? Error Details:\n%s") % error
> + self.ui.error(None, summary, text)
> + return

Can you clarify the rationale for creating a software-center dialog
and exiting the function here? I would have thought you'd want to have
graphs of the occurrence of the authentication agent not running, as
it may be more often than we suspect and be evidence of a larger
problem. We won't know until we measure though.

> + # show a apport recoverable error dialog to the user as we want
> + # to know about these issues
> + self._call_apport_recoverable_error(
> + _("There was a error submitting the transaction"), error)

Grammar nit: an before vowel sounds. So it's "there was an error."

> + # mvo: I don't think we need to send "Package\0software-center",
> + # apport should figure this out itself
> + data = "DialogBody\0%(text)s\0Traceback\0%(error)s" % {
> + 'text': text,
> + 'error': error,
> + }
> + # this will be quick as it just writes the report file and the
> + # helper is spawned by apport asyncronsoly

Correct, but to clarify: the report is written, update-notifier has an
inotify watch on /var/crash and it then calls apport to process unseen
reports.

> + APPORT_RECOVERABLE_ERROR = "/usr/share/apport/recoverable_problem"
> + p = Popen(
> + [APPORT_RECOVERABLE_ERROR], stdin=PIPE, stdout=PIPE, stderr=PIPE)
> +
> + (stdout, stderr) = p.communicate(input=data)
> + return_code = p.wait()

subprocess.communicate() calls wait():
Lib/subprocess.py:communicate:751

So you could just grab it off the pid member. There's no harm in
calling wait() again though, so feel free to leave that.

> === modified file 'tests/test_aptd.py'
> --- tests/test_aptd.py 2012-06-28 08:51:44 +0000
> +++ tests/test_aptd.py 2012-08-22 07:56:18 +0000
> @@ -121,5 +143,15 @@
> + @patch("softwarecenter.backend.installbackend_impl.aptd.Popen")
> + def test_recoverable_error(self, mock_popen):
> + mock_popen_instance = Mock()
> + mock_popen_instance.communicate.return_value = ("stdout", "stderr")
> + mock_popen_instance.wait.return_value = 0
> + mock_popen.return_value = mock_popen_instance
> + self.aptd._call_apport_recoverable_error("msg", "error-error")
> + mock_popen_instance.communicate.assert_called()

You could expand this to check the arguments to Popen and put a
StringIO on stdin to ensure that you're getting three nuls and the
right key-value pairs in there, if you'd like.

Looks great!