Merge lp:~liuyq0307/lava-dashboard/add-measurement-for-json into lp:lava-dashboard

[Yongqin Liu]

add measurement item to the results of json about "bundles/(?P<content_sha1>[0-9a-z]+)/json$" url.
Doing this can help we display the measurement of each test result on the android-build page.

and modify according review comments: using models to do sorting and filtering
put the logic of getting measurement information to view function.

[Michael Hudson-Doyle (mwhudson)]

Looks ok, probably better to filter for the test results with measurements in the db rather than in Python. Could even do the sorting that way too...

[Yongqin Liu (liuyq0307)]

> Looks ok, probably better to filter for the test results with measurements in
> the db rather than in Python. Could even do the sorting that way too...

Thanks, I have modified according to your comment.

[Zygmunt Krynicki (zyga)]

17 + result['measurements'] = sorted(measurements)
This line is pointless, broken or both: you are sorting a list of dictionaries. This gives them semi-random order. What you probably want instead is to sort in the database (as you already do) and not sort here or not sort in the database and sort here with an explicit key function.

Overall the code is okay, a little verbose. The bigger question is: why is this added here? I don't see why this is in the summary function (which is going away as we have denormalized models now). I'd rather see this in a view that uses this logic.

review: Disapprove

[Yongqin Liu (liuyq0307)]

> Overall the code is okay, a little verbose. The bigger question is: why is
> this added here? I don't see why this is in the summary function (which is
> going away as we have denormalized models now). I'd rather see this in a view
> that uses this logic.

for bug https://bugs.launchpad.net/lava-android-test/+bug/877859,
we want to make the result display on android-build page look like this:
https://launchpadlibrarian.net/83294833/MeasurementDisplay.png
so, i use the original "/json" url and the view function bundle_json to get the data necessary.
and this make the modification a little less.

[Zygmunt Krynicki (zyga)]

Shorter != better.

Mwhudson, what is your opinion on this?

[Michael Hudson-Doyle (mwhudson)]

Ah, I agree with Zygmunt that it doesn't really make sense to change get_summary_results for this -- I misread the patch earlier, sorry. Can't you do this by just changing the bundle_json view method?

[Yongqin Liu (liuyq0307)]

> Ah, I agree with Zygmunt that it doesn't really make sense to change
> get_summary_results for this -- I misread the patch earlier, sorry. Can't you
> do this by just changing the bundle_json view method?

sorry, i am not very clear about the coding guidelines for models.
Do you mean following for this modification:
1. delete the modification I did in models.py
2. add the same logic into the bundle_json view method in views.py
does this mean that it's better not to add logic to models.py,
and it's better to move the logic of get_summary_results out of models.py?

[Zygmunt Krynicki (zyga)]

Wysłane z iPhone'a

Dnia 25 paź 2011 o godz. 04:41 Yongqin Liu <email address hidden> napisał(a):

>> Ah, I agree with Zygmunt that it doesn't really make sense to change
>> get_summary_results for this -- I misread the patch earlier, sorry. Can't you
>> do this by just changing the bundle_json view method?
>
> sorry, i am not very clear about the coding guidelines for models.
> Do you mean following for this modification:
> 1. delete the modification I did in models.py
> 2. add the same logic into the bundle_json view method in views.py
> does this mean that it's better not to add logic to models.py,
> and it's better to move the logic of get_summary_results out of models.py?

Both models and views should have logic. The difference is in the details. Here You are changing an existing method. Since the change is only needed by one view I think that code should be in the view function instead.

At the very least it would be better to add a new method to the model than to change an existing one.

HTH
ZK
> --
> https://code.launchpad.net/~liuyq0307/lava-dashboard/add-measurement-for-json/+merge/80194
> You are reviewing the proposed merge of lp:~liuyq0307/lava-dashboard/add-measurement-for-json into lp:lava-dashboard.

[Zygmunt Krynicki (zyga)]

+1

review: Approve

=== modified file 'dashboard_app/views.py'
--- dashboard_app/views.py	2011-10-06 12:03:00 +0000
+++ dashboard_app/views.py	2011-10-25 09:16:26 +0000
@@ -187,10 +187,21 @@
     bundle = bundle_stream.bundles.get(content_sha1=content_sha1)
     test_runs = []
     for test_run in bundle.test_runs.all():
+        results = test_run.get_summary_results()
+
+        measurements = [{'item': str(item.test_case),
+                           'measurement': str(item.measurement),
+                           'units': str(item.units)
+                          }
+                for item in test_run.test_results.filter(
+                            measurement__isnull=False).
+                        order_by('test_case__test_case_id')]
+        results['measurements'] = measurements
+
         test_runs.append({
             'name': test_run.test.test_id,
             'url': request.build_absolute_uri(test_run.get_absolute_url()),
-            'results': test_run.get_summary_results()
+            'results': results
             })
     json_text = json.dumps({
         'test_runs':test_runs,