ui: don't rely on Dependencies to know if collect_info() has been run

[schopin-pro]

Ever since we've started collecting deps info at crash file creation, that code path was essentially dead, and that resulted in the _MarkForUpload field never touched in some circumstances.

This quick workaround is to actually check for that internal field instead, as it's only written in collect_info(), but I think we'll need a better solution in the long term.

Fixes: ea64857 ("feat: Already add package info during crash")
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/apport/+bug/2038650

[codecov]

Codecov Report

Merging #224 (a16de4e) into main (81eb04e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #224   +/-   ##
=======================================
  Coverage   82.81%   82.81%           
=======================================
  Files          91       91           
  Lines       18736    18737    +1     
=======================================
+ Hits        15516    15517    +1     
  Misses       3220     3220           

Files	Coverage Δ
apport/ui.py	72.18% <100.00%> (ø)
tests/integration/test_ui.py	97.44% <100.00%> (+<0.01%)	⬆️

[murraybd]

I modified apport/ui.py locally with these changes and they worked for me i.e. a crash report was successfully uploaded to the Error Tracker.

[bdrung]

question: collect_info has a "skip if we already ran" check and returns for ProblemType != Crash and Dependencies present. Have you tested this code path?

thought: collect_info sets self.report["_MarkForUpload"] = "True" as first step. So checking for this field means "started collection" and not "finished collection". Moving setting _MarkForUpload might change that.

[schopin-pro]

You are indeed right that there's a change of behaviour for non-crash reports: now they might actually get uploaded! Except for Hang reports that are marked for upload on their own, any non-crash report that would come in with already-populated Dependencies would have been totally ignored. With this fix at least we get some data.

I haven't tested though, as I don't have the first idea how to trigger non-crash reports, and crafting bogus reports would hardly prove anything.

As far as moving _MarkForUpload, I say don't bother. The entire thing is an organically grown wonder of complexity, just moving a single thing around is bound to have hideous consequences. Let's just take this small win first, and then work on clearing it all up in a subsequent step.

[bdrung]

Besides the Crash problem type there are KernelOops, KernelCrash, Package (for failed install/upgrade of a software package), and Bug (for general bug reports). We should test those as well (and ensure we have test coverage for them as well).

I'll merge this PR since it is a first step in the right direction.

[murraybd]

Besides the Crash problem type there are KernelOops, KernelCrash, Package (for failed install/upgrade of a software package), and Bug (for general bug reports). We should test those as well (and ensure we have test coverage for them as well).

There is also the RecoverableProblem problem type which should also be considered and tested.