From 6c170a14abfe6390ea58873d1e110bd2a5e87805 Mon Sep 17 00:00:00 2001 From: Ilya Shakhat Date: Fri, 6 Dec 2013 15:49:41 +0400 Subject: [PATCH] Implemented reviews stats report * Calculate whether mark is in disagreement state (differs from later core's mark) * Add report that shows stats for last specified number of days * Don't treat 'Approve' marks as review marks * Fix table styles to show ordering arrows Implements blueprint review-disagreements Change-Id: I9590d69242b533ba1336384c43a9daa3dfc4e926 --- dashboard/decorators.py | 32 ++++-- dashboard/helpers.py | 7 +- dashboard/reports.py | 13 ++- dashboard/static/css/jquery.dataTables.css | 5 +- dashboard/static/js/stackalytics-ui.js | 4 +- dashboard/templates/overview.html | 8 +- dashboard/templates/reports/reviews.html | 126 +++++++++++++++++++++ dashboard/web.py | 15 ++- stackalytics/processor/record_processor.py | 45 ++++++++ tests/unit/test_record_processor.py | 56 +++++++++ 10 files changed, 285 insertions(+), 26 deletions(-) create mode 100644 dashboard/templates/reports/reviews.html diff --git a/dashboard/decorators.py b/dashboard/decorators.py index bf8a54ca2..818c71559 100644 --- a/dashboard/decorators.py +++ b/dashboard/decorators.py @@ -157,33 +157,41 @@ def aggregate_filter(): result[record[param_id]]['metric'] += record['loc'] def mark_filter(result, record, param_id): - value = record['value'] result_by_param = result[record[param_id]] - result_by_param['metric'] += 1 - - if value in result_by_param: - result_by_param[value] += 1 + if record['type'] == 'APRV': + value = 'A' else: - result_by_param[value] = 1 + value = record['value'] + result_by_param['metric'] += 1 + result_by_param[value] = result_by_param.get(value, 0) + 1 + + if record.get('x'): + result_by_param['disagreements'] = ( + result_by_param.get('disagreements', 0) + 1) def mark_finalize(record): - new_record = {} - for key in ['id', 'metric', 'name']: - new_record[key] = record[key] + new_record = record.copy() positive = 0 mark_distribution = [] - for key in [-2, -1, 1, 2]: + for key in [-2, -1, 1, 2, 'A']: if key in record: if key in [1, 2]: positive += record[key] mark_distribution.append(str(record[key])) else: mark_distribution.append('0') + new_record[key] = 0 + positive_ratio = ' (%.1f%%)' % ( + (positive * 100.0) / record['metric']) new_record['mark_ratio'] = ( - '|'.join(mark_distribution) + - ' (%.1f%%)' % ((positive * 100.0) / record['metric'])) + '|'.join(mark_distribution) + positive_ratio) + new_record['positive_ratio'] = positive_ratio + new_record['disagreements'] = record.get('disagreements', 0) + new_record['disagreement_ratio'] = '%.1f%%' % ( + (record.get('disagreements', 0) * 100.0) / record['metric'] + ) return new_record metric_param = (flask.request.args.get('metric') or diff --git a/dashboard/helpers.py b/dashboard/helpers.py index 2623f817d..b48ea8121 100644 --- a/dashboard/helpers.py +++ b/dashboard/helpers.py @@ -119,7 +119,7 @@ def get_activity(records, start_record=0, def get_contribution_summary(records): - marks = dict((m, 0) for m in [-2, -1, 0, 1, 2]) + marks = dict((m, 0) for m in [-2, -1, 0, 1, 2, 'A']) commit_count = 0 loc = 0 drafted_blueprint_count = 0 @@ -132,7 +132,10 @@ def get_contribution_summary(records): commit_count += 1 loc += record['loc'] elif record['record_type'] == 'mark': - marks[record['value']] += 1 + if record['type'] == 'APRV': + marks['A'] += 1 + else: + marks[record['value']] += 1 elif record['record_type'] == 'email': email_count += 1 elif record['record_type'] == 'bpd': diff --git a/dashboard/reports.py b/dashboard/reports.py index f6730f211..e7d24c65b 100644 --- a/dashboard/reports.py +++ b/dashboard/reports.py @@ -78,7 +78,7 @@ def _process_stat(data, key, time_now): } -@blueprint.route('/reviews/') +@blueprint.route('/reviews//open') @decorators.templated() @decorators.exception_handler() def open_reviews(module): @@ -115,6 +115,17 @@ def open_reviews(module): } +@blueprint.route('/reviews//') +@decorators.templated() +@decorators.exception_handler() +def reviews(module, days): + return { + 'module': module, + 'days': days, + 'start_date': int(time.time()) - int(days) * 24 * 60 * 60 + } + + def _get_punch_card_data(records): punch_card_raw = [] # matrix days x hours for wday in xrange(0, 7): diff --git a/dashboard/static/css/jquery.dataTables.css b/dashboard/static/css/jquery.dataTables.css index 13c6d920a..211ac511b 100644 --- a/dashboard/static/css/jquery.dataTables.css +++ b/dashboard/static/css/jquery.dataTables.css @@ -16,7 +16,6 @@ border-right: 1px solid #CCC; background-color: #F0F3FA; text-align: left; font-weight: normal; -background-image: none; border-bottom: 1px dotted #CECECE; text-shadow: 1px 1px 0px white; padding: 5px 6px; @@ -45,10 +44,10 @@ table.dataTable td.dataTables_empty { table.dataTable tr.odd { background-color: #fbfafa; } table.dataTable tr.even { background-color: white; } -table.dataTable tr.odd td.sorting_1 { background-color: #eef1f4; border-right: 1px solid #CCC; } +table.dataTable tr.odd td.sorting_1 { background-color: #eef1f4; } table.dataTable tr.odd td.sorting_2 { background-color: #DADCFF; } table.dataTable tr.odd td.sorting_3 { background-color: #E0E2FF; } -table.dataTable tr.even td.sorting_1 { background-color: #f8f9fa; border-right: 1px solid #CCC; } +table.dataTable tr.even td.sorting_1 { background-color: #f8f9fa; } table.dataTable tr.even td.sorting_2 { background-color: #F2F3FF; } table.dataTable tr.even td.sorting_3 { background-color: #F9F9FF; } diff --git a/dashboard/static/js/stackalytics-ui.js b/dashboard/static/js/stackalytics-ui.js index 115bed985..ea08d2156 100644 --- a/dashboard/static/js/stackalytics-ui.js +++ b/dashboard/static/js/stackalytics-ui.js @@ -121,8 +121,10 @@ function renderTableAndChart(url, container_id, table_id, chart_id, link_param, } } - if (data[i].core) { + if (data[i].core == "master") { data[i].link += ' ✻' + } else if (data[i].core) { + data[i].link += " ✬ " + data[i].core + ""; } tableData.push(data[i]); diff --git a/dashboard/templates/overview.html b/dashboard/templates/overview.html index adce15d31..027490243 100644 --- a/dashboard/templates/overview.html +++ b/dashboard/templates/overview.html @@ -181,13 +181,15 @@

Contribution Summary

Total commits: ${commit_count}
Total LOC: ${loc}
-
Review stat (-2, -1, +1, +2): ${marks["-2"]}, ${marks["-1"]}, ${marks["1"]}, ${marks["2"]}
+
Review stat (-2, -1, +1, +2, A): ${marks["-2"]}, ${marks["-1"]}, ${marks["1"]}, ${marks["2"]}, ${marks["A"]}
{% endraw %}
Draft Blueprints: ${drafted_blueprint_count}
Completed Blueprints: ${completed_blueprint_count}
Emails: ${email_count}
{% if module %} - + + + {% endif %} {% if company %} @@ -273,7 +275,7 @@ # Engineer {% if show_review_ratio %} - -2|-1|+1|+2 (+/- ratio) + -2|-1|+1|+2|A (+/- ratio) {% endif %} {{ metric_label }} diff --git a/dashboard/templates/reports/reviews.html b/dashboard/templates/reports/reviews.html new file mode 100644 index 000000000..3a4dab22f --- /dev/null +++ b/dashboard/templates/reports/reviews.html @@ -0,0 +1,126 @@ +{% extends "reports/base_report.html" %} + +{% block title %} +Reviews for the last {{ days }} days in {{ module }} +{% endblock %} + +{% block scripts %} + + + + + + +{% endblock %} + +{% block content %} +

Reviews for the last {{ days }} days in {{ module }}

+ + + + + + + + + + + + + + + + + + + +
#EngineerReviews-2-1+1+2A(+/- %)DisagreementsRatio
+ +
+ +{% endblock %} \ No newline at end of file diff --git a/dashboard/web.py b/dashboard/web.py index 1add61030..984e6d8a0 100644 --- a/dashboard/web.py +++ b/dashboard/web.py @@ -126,11 +126,18 @@ def get_modules(records, metric_filter, finalize_handler): def get_engineers(records, metric_filter, finalize_handler): exclude = flask.request.args.get('exclude') - module = parameters.get_single_parameter({}, 'module') + modules_names = parameters.get_parameter({}, 'module', 'modules') + modules = vault.resolve_modules(modules_names) def filter_core_users(record): user = vault.get_user_from_runtime_storage(record['id']) - is_core = (module, 'master') in user['core'] + is_core = False + for (module, branch) in user['core']: + if module in modules: + is_core = branch + if branch == 'master': # we need master, but stables are ok + break + if exclude: if ((exclude == 'non-core' and is_core) or (exclude == 'core' and not is_core)): @@ -404,7 +411,7 @@ def timeline(records, **kwargs): start_date = release_start_date = _get_date(kwargs, 'start_date') end_date = release_end_date = _get_date(kwargs, 'end_date') else: - release = releases[release_name[0]] + release = releases[release_name] start_date = release_start_date = utils.timestamp_to_week( release['start_date']) end_date = release_end_date = utils.timestamp_to_week( @@ -439,7 +446,7 @@ def timeline(records, **kwargs): if week in weeks: week_stat_loc[week] += handler(record) week_stat_commits[week] += 1 - if 'all' in release_name or record['release'] in release_name: + if 'all' == release_name or record['release'] == release_name: week_stat_commits_hl[week] += 1 # form arrays in format acceptable to timeline plugin diff --git a/stackalytics/processor/record_processor.py b/stackalytics/processor/record_processor.py index aa79cce23..a9bbf9a09 100644 --- a/stackalytics/processor/record_processor.py +++ b/stackalytics/processor/record_processor.py @@ -248,6 +248,7 @@ class RecordProcessor(object): patch_sets = record.get('patchSets', []) review['updated_on'] = review['date'] + review['patch_count'] = len(patch_sets) if patch_sets: patch = patch_sets[-1] if 'approvals' in patch: @@ -293,6 +294,7 @@ class RecordProcessor(object): mark['module'] = module mark['branch'] = branch mark['review_id'] = review_id + mark['patch'] = int(patch['number']) self._update_record_and_user(mark) @@ -532,6 +534,49 @@ class RecordProcessor(object): if user['core'] != core_old: utils.store_user(self.runtime_storage_inst, user) + def _marks_with_disagreement(self): + LOG.debug('Process marks to find disagreements') + marks_per_patch = {} + for record in self.runtime_storage_inst.get_all_records(): + if record['record_type'] == 'mark' and record['type'] == 'CRVW': + review_id = record['review_id'] + patch_number = record['patch'] + if (review_id, patch_number) in marks_per_patch: + marks_per_patch[(review_id, patch_number)].append(record) + else: + marks_per_patch[(review_id, patch_number)] = [record] + + cores = dict([(user['user_id'], user) + for user in self.runtime_storage_inst.get_all_users() + if user['core']]) + + for key, marks in marks_per_patch.iteritems(): + if len(marks) < 2: + continue + + core_mark = 0 + for mark in sorted(marks, key=lambda x: x['date'], reverse=True): + + if core_mark == 0: + user_id = mark['user_id'] + if user_id in cores: + user = cores[user_id] + if (mark['module'], mark['branch']) in user['core']: + # mark is from core engineer + core_mark = mark['value'] + continue + + disagreement = (core_mark != 0) and ( + (core_mark < 0 < mark['value']) or + (core_mark > 0 > mark['value'])) + old_disagreement = mark.get('x') + mark['x'] = disagreement + if old_disagreement != disagreement: + yield mark + def finalize(self): self.runtime_storage_inst.set_records( self._get_records_for_users_to_update()) + + self.runtime_storage_inst.set_records( + self._marks_with_disagreement()) diff --git a/tests/unit/test_record_processor.py b/tests/unit/test_record_processor.py index 4e396b545..6a101062b 100644 --- a/tests/unit/test_record_processor.py +++ b/tests/unit/test_record_processor.py @@ -12,6 +12,7 @@ # implied. # See the License for the specific language governing permissions and # limitations under the License. +import itertools import mock import testtools @@ -762,6 +763,61 @@ class TestRecordProcessor(testtools.TestCase): review2 = runtime_storage_inst.get_by_primary_key('I222') self.assertEquals(1, review2['review_number']) + def test_mark_disagreement(self): + record_processor_inst = self.make_record_processor( + users=[ + {'user_id': 'john_doe', + 'launchpad_id': 'john_doe', + 'user_name': 'John Doe', + 'emails': ['john_doe@ibm.com'], + 'core': [('nova', 'master')], + 'companies': [{'company_name': 'IBM', 'end_date': 0}]} + ], + ) + runtime_storage_inst = record_processor_inst.runtime_storage_inst + runtime_storage_inst.set_records(record_processor_inst.process([ + {'record_type': 'review', + 'id': 'I1045730e47e9e6ad31fcdfbaefdad77e2f3b2c3e', + 'subject': 'Fix AttributeError in Keypair._add_details()', + 'owner': {'name': 'John Doe', + 'email': 'john_doe@ibm.com', + 'username': 'john_doe'}, + 'createdOn': 1379404951, + 'module': 'nova', + 'branch': 'master', + 'patchSets': [ + {'number': '1', + 'revision': '4d8984e92910c37b7d101c1ae8c8283a2e6f4a76', + 'ref': 'refs/changes/16/58516/1', + 'uploader': { + 'name': 'Bill Smith', + 'email': 'bill@smith.to', + 'username': 'bsmith'}, + 'createdOn': 1385470730, + 'approvals': [ + {'type': 'CRVW', 'description': 'Code Review', + 'value': '1', 'grantedOn': 1385478465, + 'by': { + 'name': 'Homer Simpson', + 'email': 'hsimpson@gmail.com', + 'username': 'homer'}}, + {'type': 'CRVW', 'description': 'Code Review', + 'value': '-2', 'grantedOn': 1385478466, + 'by': { + 'name': 'John Doe', + 'email': 'john_doe@ibm.com', + 'username': 'john_doe'}} + ] + }]} + ])) + record_processor_inst.finalize() + + marks = list([r for r in runtime_storage_inst.get_all_records() + if r['record_type'] == 'mark']) + homer_mark = next(itertools.ifilter( + lambda x: x['date'] == 1385478465, marks), None) + self.assertTrue(homer_mark['x']) # disagreement + # update records def _generate_record_commit(self):