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

On Wed, Aug 22, 2012 at 11:10:00AM -0000, Evan Dandrea wrote:
> 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.

You are right, it should be meassured, I now modified the code to do
that.

What is your opinion on showing the internal dialog still in addition
to fire of the report to apport? The assumption here is that the user
might not have update-notifier runnig to pick the report up
immediately and because in this case we know what the problem is its
nice to show it to the user. Alternatively I could make this
conditional, i.e. if update-notifier is availalbe rely on that and if
not display a internal dialog. WDYT?

> > + # 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."

Fixed.

> > + # 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.

I updated the comment in the code to make this a bit clearer.

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

Good point :) I simply use p.returncode now.

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

Another good suggestion, I added this now too.

Thanks again for this review and I look forward hearing your
thoughts. If you feel like its ready, don't hesitate to add approved
as it will speed up the process to land it in quantal.

Cheers,
 Michael