diff --git a/forwardport/data/views.xml b/forwardport/data/views.xml index 01af17e2..be8480d9 100644 --- a/forwardport/data/views.xml +++ b/forwardport/data/views.xml @@ -136,31 +136,6 @@ t-attf-title="@{{pr.reviewed_by.github_login}}"/> - -
forward-port of
-
- - - - - DETACHED () - -
- - -
forward-ports
-
- diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index b00bf520..c69b3a3c 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -56,7 +56,8 @@ class re_matches: return repr(str(self)) def seen(env, pr, users): - return users['user'], f'[Pull request status dashboard]({to_pr(env, pr).url}).' + url = to_pr(env, pr).url + return users['user'], f'[![Pull request status dashboard]({url}.png)]({url})' def make_basic( env, diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index 9799907d..13bed3dc 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -12,6 +12,7 @@ 'data/runbot_merge.pull_requests.feedback.template.csv', 'views/res_partner.xml', 'views/runbot_merge_project.xml', + 'views/batch.xml', 'views/mergebot.xml', 'views/queues.xml', 'views/configuration.xml', diff --git a/runbot_merge/controllers/dashboard.py b/runbot_merge/controllers/dashboard.py index b06ee8bf..0e19401c 100644 --- a/runbot_merge/controllers/dashboard.py +++ b/runbot_merge/controllers/dashboard.py @@ -1,13 +1,26 @@ # -*- coding: utf-8 -*- +from __future__ import annotations + +import base64 import collections +import colorsys +import hashlib +import io import json +import math import pathlib +from email.utils import formatdate +from itertools import chain, product +from typing import Tuple, cast, Mapping import markdown import markupsafe import werkzeug.exceptions +import werkzeug.wrappers +from PIL import Image, ImageDraw, ImageFont from odoo.http import Controller, route, request +from odoo.tools import file_open LIMIT = 20 class MergebotDashboard(Controller): @@ -79,8 +92,8 @@ class MergebotDashboard(Controller): 'entries': entries, }) - @route('///pull/', auth='public', type='http', website=True, sitemap=False) - def pr(self, org, repo, pr): + @route('///pull/', auth='public', type='http', website=True, sitemap=False) + def pr(self, org, repo, pr, png): pr_id = request.env['runbot_merge.pull_requests'].sudo().search([ ('repository.name', '=', f'{org}/{repo}'), ('number', '=', int(pr)), @@ -90,6 +103,9 @@ class MergebotDashboard(Controller): if not pr_id.repository.group_id <= request.env.user.groups_id: raise werkzeug.exceptions.NotFound() + if png: + return raster_render(pr_id) + st = {} if pr_id.statuses: # normalise `statuses` to map to a dict @@ -102,3 +118,218 @@ class MergebotDashboard(Controller): 'merged_head': json.loads(pr_id.commits_map).get(''), 'statuses': st }) + +def raster_render(pr): + default_headers = { + 'Content-Type': 'image/png', + 'Last-Modified': formatdate(), + # - anyone can cache the image, so public + # - crons run about every minute so that's how long a request is fresh + # - if the mergebot can't be contacted, allow using the stale response (no must-revalidate) + # - intermediate caches can recompress the PNG if they want (pillow is not a very good PNG generator) + # - the response is mutable even during freshness, technically (as there + # is no guarantee the freshness window lines up with the cron, plus + # some events are not cron-based) + # - maybe don't allow serving the stale image *while* revalidating? + # - allow serving a stale image for a day if the server returns 500 + 'Cache-Control': 'public, max-age=60, stale-if-error=86400', + } + if if_none_match := request.httprequest.headers.get('If-None-Match'): + # just copy the existing value out if we received any + default_headers['ETag'] = if_none_match + + # weak validation: check the latest modification date of all objects involved + project, repos, branches, genealogy = pr.env.ref('runbot_merge.dashboard-pre')\ + ._run_action_code_multi({'pr': pr}) + + # last-modified should be in RFC2822 format, which is what + # email.utils.formatdate does (sadly takes a timestamp but...) + last_modified = formatdate(max(( + o.write_date + for o in chain( + project, + repos, + branches, + genealogy, + genealogy.all_prs | pr, + ) + )).timestamp()) + # The (304) response must not contain a body and must include the headers + # that would have been sent in an equivalent 200 OK response + headers = {**default_headers, 'Last-Modified': last_modified} + if request.httprequest.headers.get('If-Modified-Since') == last_modified: + return werkzeug.wrappers.Response(status=304, headers=headers) + + with file_open('web/static/fonts/google/Open_Sans/Open_Sans-Regular.ttf', 'rb') as f: + font = ImageFont.truetype(f, size=16, layout_engine=0) + f.seek(0) + supfont = ImageFont.truetype(f, size=10, layout_engine=0) + with file_open('web/static/fonts/google/Open_Sans/Open_Sans-Bold.ttf', 'rb') as f: + bold = ImageFont.truetype(f, size=16, layout_engine=0) + + batches = pr.env.ref('runbot_merge.dashboard-prep')._run_action_code_multi({ + 'pr': pr, + 'repos': repos, + 'branches': branches, + 'genealogy': genealogy, + }) + + # getbbox returns (left, top, right, bottom) + + rows = {b: font.getbbox(b.name)[3] for b in branches} + rows[None] = max(bold.getbbox(r.name)[3] for r in repos) + + columns = {r: bold.getbbox(r.name)[2] for r in repos} + columns[None] = max(font.getbbox(b.name)[2] for b in branches) + + etag = hashlib.sha256(f"(P){pr.id},{pr.repository.id},{pr.target.id}".encode()) + # repos and branches should be in a consistent order so can just hash that + etag.update(''.join(f'(R){r.name}' for r in repos).encode()) + etag.update(''.join(f'(T){b.name},{b.active}' for b in branches).encode()) + # and product of deterministic iterations should be deterministic + for r, b in product(repos, branches): + ps = batches[r, b] + etag.update(f"(B){ps['state']},{ps['detached']},{ps['active']}".encode()) + # technically label (state + blocked) does not actually impact image + # render (though subcomponents of state do) however blocked is useful + # to force an etag miss so keeping it + # TODO: blocked includes draft & merge method, maybe should change looks? + etag.update(''.join( + f"(PS){p['label']},{p['closed']},{p['number']},{p['checked']},{p['reviewed']},{p['attached']}" + for p in ps['prs'] + ).encode()) + + w = h = 0 + for p in ps['prs']: + _, _, ww, hh = font.getbbox(f" #{p['number']}") + w += ww + supfont.getbbox(' '.join(filter(None, [ + 'error' if p['pr'].error else '', + '' if p['checked'] else 'unchecked', + '' if p['reviewed'] else 'unreviewed', + '' if p['attached'] else 'detached', + ])))[2] + h = max(hh, h) + rows[b] = max(rows.get(b, 0), h) + columns[r] = max(columns.get(r, 0), w) + + etag = headers['ETag'] = base64.b32encode(etag.digest()).decode() + if if_none_match == etag: + return werkzeug.wrappers.Response(status=304, headers=headers) + + pad_w, pad_h = 20, 5 + image_height = sum(rows.values()) + 2 * pad_h * len(rows) + image_width = sum(columns.values()) + 2 * pad_w * len(columns) + im = Image.new("RGB", (image_width+1, image_height+1), color='white') + draw = ImageDraw.Draw(im, 'RGB') + draw.font = font + + # for reasons of that being more convenient we store the bottom of the + # current row, so getting the top edge requires subtracting h + w = left = bottom = 0 + for b, r in product(chain([None], branches), chain([None], repos)): + left += w + + opacity = 1.0 if b is None or b.active else 0.5 + background = BG['info'] if b == pr.target or r == pr.repository else BG[None] + w, h = columns[r] + 2 * pad_w, rows[b] + 2 * pad_h + + if r is None: # branch cell in row + left = 0 + bottom += h + if b: + draw.rectangle( + (left + 1, bottom - h + 1, left+w - 1, bottom - 1), + background, + ) + draw.text( + (left + pad_w, bottom - h + pad_h), + b.name, + fill=blend(TEXT, opacity, over=background), + ) + elif b is None: # repo cell in top row + draw.rectangle((left + 1, bottom - h + 1, left+w - 1, bottom - 1), background) + draw.text((left + pad_w, bottom - h + pad_h), r.name, fill=TEXT, font=bold) + # draw the bottom-right edges of the cell + draw.line([ + (left, bottom), # bottom-left + (left + w, bottom), # bottom-right + (left+w, bottom-h) # top-right + ], fill=(172, 176, 170)) + if r is None or b is None: + continue + + ps = batches[r, b] + + bgcolor = BG[ps['state']] + if pr in ps['pr_ids']: + bgcolor = lighten(bgcolor, by=-0.05) + background = blend(bgcolor, opacity, over=background) + draw.rectangle((left + 1, bottom - h + 1, left+w - 1, bottom - 1), background) + + top = bottom - h + pad_h + offset = left + pad_w + for p in ps['prs']: + label = f"#{p['number']}" + foreground = blend((39, 110, 114), opacity, over=background) + draw.text((offset, top), label, fill=foreground) + x, _, ww, hh = font.getbbox(label) + if p['closed']: + draw.line([ + (offset+x, top + hh - hh/3), + (offset+x+ww, top + hh - hh/3), + ], fill=foreground) + offset += ww + if not p['attached']: + # overdraw top border to mark the detachment + draw.line([(left, bottom-h), (left+w, bottom-h)], fill=ERROR) + for attribute in filter(None, [ + 'error' if p['pr'].error else '', + '' if p['checked'] else 'unchecked', + '' if p['reviewed'] else 'unreviewed', + '' if p['attached'] else 'detached', + ]): + label = f' {attribute}' + draw.text((offset, top), label, + fill=blend(ERROR, opacity, over=background), + font=supfont) + offset += supfont.getbbox(label)[2] + offset += math.ceil(supfont.getlength(" ")) + + buffer = io.BytesIO() + im.save(buffer, 'png', optimize=True) + return werkzeug.wrappers.Response(buffer.getvalue(), headers=headers) + +Color = Tuple[int, int, int] +TEXT: Color = (102, 102, 102) +ERROR: Color = (220, 53, 69) +BG: Mapping[str | None, Color] = collections.defaultdict(lambda: (255, 255, 255), { + 'info': (217, 237, 247), + 'success': (223, 240, 216), + 'warning': (252, 248, 227), + 'danger': (242, 222, 222), +}) +def blend_single(c: int, over: int, opacity: float) -> int: + return round(over * (1 - opacity) + c * opacity) + +def blend(color: Color, opacity: float, *, over: Color = (255, 255, 255)) -> Color: + assert 0.0 <= opacity <= 1.0 + return ( + blend_single(color[0], over[0], opacity), + blend_single(color[1], over[1], opacity), + blend_single(color[2], over[2], opacity), + ) + +def lighten(color: Color, *, by: float) -> Color: + # colorsys uses values in the range [0, 1] rather than pillow/CSS-style [0, 225] + r, g, b = tuple(c / 255 for c in color) + hue, lightness, saturation = colorsys.rgb_to_hls(r, g, b) + + # by% of the way between value and 1.0 + if by >= 0: lightness += (1.0 - lightness) * by + # -by% of the way between 0 and value + else:lightness *= (1.0 + by) + + return cast(Color, tuple( + round(c * 255) + for c in colorsys.hls_to_rgb(hue, lightness, saturation) + )) diff --git a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv index d9e64d2f..5fe768ae 100644 --- a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv +++ b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv @@ -53,7 +53,7 @@ runbot_merge.failure.approved,{pr.ping}{status!r} failed on this reviewed PR.,"N pr: pull request in question status: failed status" -runbot_merge.pr.created,[Pull request status dashboard]({pr.url}).,"Initial comment on PR creation. +runbot_merge.pr.created,[![Pull request status dashboard]({pr.url}.png)]({pr.url}),"Initial comment on PR creation. pr: created pr" runbot_merge.pr.linked.not_ready,{pr.ping}linked pull request(s) {siblings} not ready. Linked PRs are not staged until all of them are ready.,"Comment when a PR is ready (approved & validated) but it is linked to other PRs which are not. diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index 1472ccd1..00886b1d 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -53,6 +53,7 @@ class Batch(models.Model): _inherit = ['mail.thread'] _parent_store = True + name = fields.Char(compute="_compute_name") target = fields.Many2one('runbot_merge.branch', store=True, compute='_compute_target') batch_staging_ids = fields.One2many('runbot_merge.staging.batch', 'runbot_merge_batch_id') staging_ids = fields.Many2many( @@ -176,6 +177,11 @@ class Batch(models.Model): def _search_open_prs(self, operator, value): return [('all_prs', operator, value), ('active', '=', True)] + @api.depends("prs.label") + def _compute_name(self): + for batch in self: + batch.name = batch.prs[:1].label or batch.all_prs[:1].label + @api.depends("all_prs.target") def _compute_target(self): for batch in self: @@ -190,7 +196,6 @@ class Batch(models.Model): else: batch.target = False - @api.depends( "merge_date", "prs.error", "prs.draft", "prs.squash", "prs.merge_method", diff --git a/runbot_merge/static/scss/runbot_merge.scss b/runbot_merge/static/scss/runbot_merge.scss index 2e6577b3..e5a18687 100644 --- a/runbot_merge/static/scss/runbot_merge.scss +++ b/runbot_merge/static/scss/runbot_merge.scss @@ -14,27 +14,30 @@ h1, h2, h3, h4, h5, h6{ margin-bottom: 0.33em; } h5 { font-size: 1em; } -.bg-success, .bg-info, .bg-warning, .bg-danger, .bg-gray-lighter { +.bg-success, .bg-info, .bg-warning, .bg-danger, .bg-gray-lighter, +.table-success, .table-info, .table-warning, .table-danger { color: inherit; } .dropdown-item, .dropdown-menu, .dropdown-menu a { color: inherit; } -.bg-success { - background-color: #dff0d8 !important; + +$mergebot-colors: ("success": #dff0d8, "danger": #f2dede, "warning": #fcf8e3, "info": #d9edf7); +@each $category, $color in $mergebot-colors { + .bg-#{$category} { + background-color: $color !important; + } + .table-#{$category} { + background-color: $color !important; + &.table-active { + background-color: scale-color($color, $lightness: -5%) !important; + } + } } .bg-unmerged { - background-color: #dcefe8 !important -} -.bg-info { - background-color: #d9edf7 !important; -} -.bg-warning { - background-color: #fcf8e3 !important; -} -.bg-danger { - background-color: #f2dede !important; + background-color: #f8f0e3 !important } + .list-inline { margin-bottom: 10px; } @@ -121,3 +124,16 @@ dl.runbot-merge-fields { // works better for the left edge of the *box* @extend .border-left; } + +// batches sequence table in PR dashboard: mostly uses (customised) bootstrap +// but some of the style is bespoke because inline styles don't work well with +// CSP +.closed { + text-decoration: line-through; +} +tr.inactive { + opacity: 0.5; +} +td.detached { + border-top: 2px solid map-get($theme-colors, "danger"); +} diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index e47ff180..3b616cb8 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -53,7 +53,6 @@ def test_trivial_flow(env, repo, page, users, config): )) == { 'label': f"{config['github']['owner']}:other", 'head': c1, - 'target': 'master', } with repo: diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py index 08cbf117..ed970974 100644 --- a/runbot_merge/tests/test_disabled_branch.py +++ b/runbot_merge/tests/test_disabled_branch.py @@ -59,12 +59,9 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf assert staging_id.reason == "Target branch deactivated by 'admin'." p = pr_page(page, pr) - target = dict(zip( - (e.text for e in p.cssselect('dl.runbot-merge-fields dt')), - (p.cssselect('dl.runbot-merge-fields dd')) - ))['target'] - assert target.text_content() == 'other (inactive)' - assert target.get('class') == 'text-muted bg-warning' + [target] = p.cssselect('table tr.bg-info') + assert 'inactive' in target.classes + assert target[0].text_content() == "other" assert pr.comments == [ (users['reviewer'], "hansen r+"), diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 2691a21e..42043f78 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -1109,11 +1109,9 @@ def test_multi_project(env, make_repo, setreviewers, users, config, assert pr1.comments == [ (users['reviewer'], 'hansen r+'), - (users['user'], f'[Pull request status dashboard]({pr1_id.url}).'), - ] - assert pr2.comments == [ - (users['user'], f'[Pull request status dashboard]({pr2_id.url}).'), + seen(env, pr1, users), ] + assert pr2.comments == [seen(env, pr2, users)] def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): """ Tests the freeze wizard feature (aside from the UI): diff --git a/runbot_merge/views/batch.xml b/runbot_merge/views/batch.xml new file mode 100644 index 00000000..89fc3902 --- /dev/null +++ b/runbot_merge/views/batch.xml @@ -0,0 +1,59 @@ + + + Batch form + runbot_merge.batch + +
+ +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+ + + diff --git a/runbot_merge/views/mergebot.xml b/runbot_merge/views/mergebot.xml index 46e87ab8..88b33f4e 100644 --- a/runbot_merge/views/mergebot.xml +++ b/runbot_merge/views/mergebot.xml @@ -131,49 +131,41 @@

- - () + + (blocked: ) + + () +

- - - - - - - - - - + + - - - - + + + + - - + - - - + @@ -290,6 +282,15 @@ + + + +