feat: staged-action UX with single Perform Deploy trigger

mekarpeles · mekarpeles · commit e1a057fdab35 · 2026-04-02T15:46:14.000-06:00
Replaces immediate-action model with a staged workflow:
- Add PR(s): new rows highlighted blue until next deploy
- Fetch Latest: queues a commit upgrade (strikethrough + ↑↑), not applied yet
- Enable/Disable: stages active state change (green/red rows)
- Remove: immediately removes row from state (no deploy needed)
- Perform Deploy: only action that triggers Jenkins; applies all pending
  changes atomically, records last_deploy_at, and redirects with a
  Jenkins progress link

State file format updated from bare array to
{"last_deploy_at":"","prs":[...]} with backward-compat read.
Shell script updated to handle new format.
diff --git a/openlibrary/plugins/openlibrary/status.py b/openlibrary/plugins/openlibrary/status.py
@@ -7,7 +7,7 @@
 import sys
 import urllib.error
 import urllib.request
-from dataclasses import dataclass
+from dataclasses import dataclass, field
 from pathlib import Path
 from typing import Any
 from urllib.parse import urlencode
@@ -27,26 +27,30 @@
 TESTING_STATE_FILE = Path('./_testing-prs.json')
 _GITHUB_API_BASE = "https://api.github.com/repos/internetarchive/openlibrary"
 _JENKINS_URL = "https://jenkins.openlibrary.org/job/testing-deploy/buildWithParameters"
+_JENKINS_JOB_URL = "https://jenkins.openlibrary.org/job/testing-deploy/"
 
 
 class status(delegate.page):
     def GET(self):
-        testing_prs = _load_testing_state()  # None if state file doesn't exist
+        testing_state = _load_testing_state()
         is_maintainer_user = _is_maintainer()
         drift_info = {}
-        if testing_prs:
+        if testing_state:
             # NOTE: makes 1-2 GitHub API calls per PR; acceptable for small testing sets
-            drift_info = {p.pr: _get_pr_drift(p) for p in testing_prs}
-        show_testing = testing_prs is not None or is_maintainer_user
+            drift_info = {p.pr: _get_pr_drift(p) for p in testing_state.prs}
+        show_testing = testing_state is not None or is_maintainer_user
+        i = web.input(deploy_triggered=None)
         return render_template(
             "status",
             status_info,
             feature_flags,
             dev_merged_status=get_dev_merged_status(),
-            testing_prs=testing_prs or [],
+            testing_state=testing_state,
             drift_info=drift_info,
             is_maintainer=is_maintainer_user,
             show_testing=show_testing,
+            deploy_triggered=bool(i.deploy_triggered),
+            jenkins_job_url=_JENKINS_JOB_URL,
         )
 
 
@@ -65,13 +69,13 @@ def POST(self):
                     pr_numbers.append(_parse_pr_number(val))
         if not pr_numbers:
             raise web.badrequest()
-        prs = _load_testing_state() or []
-        existing = {p.pr for p in prs}
+        state = _load_testing_state() or TestingState(last_deploy_at='', prs=[])
+        existing = {p.pr for p in state.prs}
         user = get_current_user()
         for pr_number in pr_numbers:
             if pr_number not in existing:
                 info = _get_pr_info(pr_number)
-                prs.append(
+                state.prs.append(
                     TestingPR(
                         pr=pr_number,
                         commit=info['head_sha'],
@@ -82,8 +86,7 @@ def POST(self):
                     )
                 )
                 existing.add(pr_number)
-        _save_testing_state(prs)
-        _trigger_rebuild()
+        _save_testing_state(state)
         raise web.seeother('/status')
 
 
@@ -95,27 +98,42 @@ def POST(self):
             raise web.unauthorized()
         i = web.input(prs=[])
         to_remove = {int(p) for p in i.prs}
-        _save_testing_state(
-            [p for p in (_load_testing_state() or []) if p.pr not in to_remove]
-        )
-        _trigger_rebuild()
+        state = _load_testing_state()
+        if state:
+            state.prs = [p for p in state.prs if p.pr not in to_remove]
+            _save_testing_state(state)
         raise web.seeother('/status')
 
 
-class status_toggle(delegate.page):
-    path = '/status/toggle'
+class status_enable(delegate.page):
+    path = '/status/enable'
 
     def POST(self):
         if not _is_maintainer():
             raise web.unauthorized()
         i = web.input(prs=[])
-        to_toggle = {int(p) for p in i.prs}
-        prs = _load_testing_state() or []
-        for p in prs:
-            if p.pr in to_toggle:
-                p.active = not p.active
-        _save_testing_state(prs)
-        _trigger_rebuild()
+        to_enable = {int(p) for p in i.prs}
+        state = _load_testing_state() or TestingState(last_deploy_at='', prs=[])
+        for p in state.prs:
+            if p.pr in to_enable:
+                p.pending_active = True
+        _save_testing_state(state)
+        raise web.seeother('/status')
+
+
+class status_disable(delegate.page):
+    path = '/status/disable'
+
+    def POST(self):
+        if not _is_maintainer():
+            raise web.unauthorized()
+        i = web.input(prs=[])
+        to_disable = {int(p) for p in i.prs}
+        state = _load_testing_state() or TestingState(last_deploy_at='', prs=[])
+        for p in state.prs:
+            if p.pr in to_disable:
+                p.pending_active = False
+        _save_testing_state(state)
         raise web.seeother('/status')
 
 
@@ -127,25 +145,35 @@ def POST(self):
             raise web.unauthorized()
         i = web.input(prs=[])
         to_update = {int(p) for p in i.prs}
-        prs = _load_testing_state() or []
-        for p in prs:
+        state = _load_testing_state() or TestingState(last_deploy_at='', prs=[])
+        for p in state.prs:
             if p.pr in to_update:
                 info = _get_pr_info(p.pr)
-                if info['head_sha']:
-                    p.commit = info['head_sha']
-        _save_testing_state(prs)
-        _trigger_rebuild()
+                if info['head_sha'] and info['head_sha'] != p.commit:
+                    p.pull_latest_sha = info['head_sha']
+        _save_testing_state(state)
         raise web.seeother('/status')
 
 
-class status_rebuild(delegate.page):
-    path = '/status/rebuild'
+class status_deploy(delegate.page):
+    path = '/status/deploy'
 
     def POST(self):
         if not _is_maintainer():
             raise web.unauthorized()
+        state = _load_testing_state() or TestingState(last_deploy_at='', prs=[])
+        # Apply all pending changes before deploying
+        for p in state.prs:
+            if p.pull_latest_sha:
+                p.commit = p.pull_latest_sha
+                p.pull_latest_sha = ''
+            if p.pending_active is not None:
+                p.active = p.pending_active
+                p.pending_active = None
+        state.last_deploy_at = datetime.datetime.now(datetime.UTC).isoformat()
+        _save_testing_state(state)
         _trigger_rebuild()
-        raise web.seeother('/status')
+        raise web.seeother('/status?deploy_triggered=1')
 
 
 @functools.cache
@@ -227,24 +255,35 @@ class TestingPR:
     title: str
     added_at: str  # ISO timestamp
     added_by: str  # OL username
+    pull_latest_sha: str = ''  # pending SHA from "Pull to Latest"; applied on deploy
+    pending_active: bool | None = None  # pending enable/disable; applied on deploy
 
     @property
     def short_commit(self) -> str:
         return self.commit[:7]
 
+    @property
+    def short_pull_latest(self) -> str:
+        return self.pull_latest_sha[:7] if self.pull_latest_sha else ''
+
     @property
     def added_date(self) -> str:
         return self.added_at[:10] if self.added_at else ''
 
     def to_dict(self) -> dict:
-        return {
+        d = {
             'pr': self.pr,
             'commit': self.commit,
             'active': self.active,
             'title': self.title,
             'added_at': self.added_at,
             'added_by': self.added_by,
         }
+        if self.pull_latest_sha:
+            d['pull_latest_sha'] = self.pull_latest_sha
+        if self.pending_active is not None:
+            d['pending_active'] = self.pending_active
+        return d
 
     @classmethod
     def from_dict(cls, d: dict) -> 'TestingPR':
@@ -255,20 +294,46 @@ def from_dict(cls, d: dict) -> 'TestingPR':
             title=d.get('title', f"PR #{d['pr']}"),
             added_at=d.get('added_at', ''),
             added_by=d.get('added_by', ''),
+            pull_latest_sha=d.get('pull_latest_sha', ''),
+            pending_active=d.get('pending_active', None),
+        )
+
+
+@dataclass
+class TestingState:
+    last_deploy_at: str  # ISO timestamp, empty if never deployed
+    prs: list[TestingPR] = field(default_factory=list)
+
+    def to_dict(self) -> dict:
+        return {
+            'last_deploy_at': self.last_deploy_at,
+            'prs': [p.to_dict() for p in self.prs],
+        }
+
+    @classmethod
+    def from_dict(cls, d: dict) -> 'TestingState':
+        return cls(
+            last_deploy_at=d.get('last_deploy_at', ''),
+            prs=[TestingPR.from_dict(p) for p in d.get('prs', [])],
         )
 
 
-def _load_testing_state() -> 'list[TestingPR] | None':
-    """Returns list of TestingPRs if state file exists, None otherwise."""
+def _load_testing_state() -> 'TestingState | None':
+    """Returns TestingState if state file exists, None otherwise."""
     if TESTING_STATE_FILE.exists():
-        return [
-            TestingPR.from_dict(d) for d in json.loads(TESTING_STATE_FILE.read_text())
-        ]
+        data = json.loads(TESTING_STATE_FILE.read_text())
+        if isinstance(data, list):
+            # Backward compat: old format was a bare array
+            return TestingState(
+                last_deploy_at='',
+                prs=[TestingPR.from_dict(d) for d in data],
+            )
+        return TestingState.from_dict(data)
     return None
 
 
-def _save_testing_state(prs: list[TestingPR]) -> None:
-    TESTING_STATE_FILE.write_text(json.dumps([p.to_dict() for p in prs], indent=2))
+def _save_testing_state(state: TestingState) -> None:
+    TESTING_STATE_FILE.write_text(json.dumps(state.to_dict(), indent=2))
     get_dev_merged_status.cache_clear()
 
 
@@ -329,7 +394,8 @@ def _trigger_rebuild() -> bool:
     token = getattr(config, 'jenkins_token', None)
     if not token:
         return False
-    prs = _load_testing_state() or []
+    state = _load_testing_state()
+    prs = state.prs if state else []
     lines = '\n'.join(f"origin pull/{p.pr}/head  # {p.title}" for p in prs if p.active)
     url = f"{_JENKINS_URL}?{urlencode({'token': token, 'GH_REPO_AND_BRANCH': lines})}"
     try:
diff --git a/openlibrary/templates/status.html b/openlibrary/templates/status.html
@@ -1,4 +1,4 @@
-$def with (status = {}, flags = {}, dev_merged_status=None, testing_prs=None, drift_info=None, is_maintainer=False, show_testing=False)
+$def with (status = {}, flags = {}, dev_merged_status=None, testing_state=None, drift_info=None, is_maintainer=False, show_testing=False, deploy_triggered=False, jenkins_job_url='')
 $# Template entry point for /status page
 $var title: $_("Open Library server status")
 
@@ -26,8 +26,18 @@
   .drift-high { background: #fff1f0; }
   .row-merged td { text-decoration: line-through; color: #999; }
   .row-inactive { opacity: 0.6; }
+  .row-new td { background: #e6f0ff; }
+  .row-pending-enable td { background: #f0fff4; }
+  .row-pending-disable td { background: #fff0f0; }
   .testing-actions { margin-top: 0.5em; }
   .testing-actions button { margin-right: 4px; }
+  .deploy-banner {
+    background: #e6ffe6;
+    border: 1px solid #5a5;
+    padding: 8px 12px;
+    margin-bottom: 1em;
+    border-radius: 4px;
+  }
 </style>
 
 <div id="contentHead">
@@ -39,13 +49,21 @@ <h1>$_("Server status")</h1>
   $if show_testing:
     <h2>$_("Testing Environment")</h2>
 
+    $if deploy_triggered:
+      <div class="deploy-banner">
+        $_("Deploy triggered!")
+        $if jenkins_job_url:
+          &mdash; <a href="$jenkins_job_url" target="_blank">$_("View Jenkins progress")</a>
+      </div>
+
     $if is_maintainer:
       <form method="POST" action="/status/add" style="margin-bottom: 1em;">
         <input type="text" name="pr" placeholder="$_('PR numbers or URLs, space/comma separated')" size="50">
-        <button type="submit">$_("Add PR")</button>
+        <button type="submit">$_("Add PR(s)")</button>
       </form>
 
-    $if testing_prs:
+    $if testing_state and testing_state.prs:
+      $ last_deploy_at = testing_state.last_deploy_at
       <form method="POST" id="testing-form">
         <table class="testing-table">
           <thead>
@@ -62,18 +80,26 @@ <h2>$_("Testing Environment")</h2>
             </tr>
           </thead>
           <tbody>
-            $for pr in testing_prs:
+            $for pr in testing_state.prs:
               $ d = (drift_info or {}).get(pr.pr, {})
               $ drift = d.get('drift', -1)
               $ head_sha = d.get('head_sha', '')
               $ merged = d.get('merged', False)
-              $ row_class = ('row-merged ' if merged else '') + ('row-inactive ' if not pr.active else '') + ('drift-high' if drift > 5 else ('drift-low' if drift > 0 else ''))
+              $ is_new = bool(last_deploy_at and pr.added_at > last_deploy_at)
+              $ row_class = 'row-merged' if merged else ('row-pending-enable' if pr.pending_active is True else ('row-pending-disable' if pr.pending_active is False else ('row-new' if is_new else ('' if pr.active else 'row-inactive'))))
+              $ row_class += (' drift-high' if drift > 5 else (' drift-low' if drift > 0 else ''))
               <tr class="$row_class.strip()">
                 $if is_maintainer:
                   <td><input type="checkbox" name="prs" value="$pr.pr"></td>
                 <td><a href="https://github.com/internetarchive/openlibrary/pull/$pr.pr">#$pr.pr</a></td>
                 <td>$pr.title</td>
-                <td><a href="https://github.com/internetarchive/openlibrary/commit/$pr.commit">$pr.short_commit</a></td>
+                <td>
+                  $if pr.pull_latest_sha:
+                    <a href="https://github.com/internetarchive/openlibrary/commit/$pr.commit"><s>$pr.short_commit</s></a>
+                    &#x2191;&#x2191; <a href="https://github.com/internetarchive/openlibrary/commit/$pr.pull_latest_sha">$pr.short_pull_latest</a>
+                  $else:
+                    <a href="https://github.com/internetarchive/openlibrary/commit/$pr.commit">$pr.short_commit</a>
+                </td>
                 <td>$head_sha</td>
                 <td>
                   $if drift == 0:
@@ -84,7 +110,11 @@ <h2>$_("Testing Environment")</h2>
                     $drift commit$("s" if drift != 1 else "") behind
                 </td>
                 <td>
-                  $if pr.active:
+                  $if pr.pending_active is True:
+                    &#x23F8; &#x2192; &#x2705;
+                  $elif pr.pending_active is False:
+                    &#x2705; &#x2192; &#x23F8;
+                  $elif pr.active:
                     &#x2705;
                   $else:
                     &#x23F8;
@@ -96,22 +126,23 @@ <h2>$_("Testing Environment")</h2>
 
         $if is_maintainer:
           <div class="testing-actions">
-            <button formaction="/status/pull-latest" type="submit">$_("Pull to Latest")</button>
-            <button formaction="/status/toggle" type="submit">$_("Toggle Active")</button>
+            <button formaction="/status/pull-latest" type="submit">$_("Fetch Latest")</button>
+            <button formaction="/status/enable" type="submit">$_("Enable")</button>
+            <button formaction="/status/disable" type="submit">$_("Disable")</button>
             <button formaction="/status/remove" type="submit">$_("Remove")</button>
           </div>
       </form>
 
       $if is_maintainer:
-        <form method="POST" action="/status/rebuild" style="margin-top: 0.5em;">
-          <button type="submit">$_("Force Rebuild")</button>
+        <form method="POST" action="/status/deploy" style="margin-top: 0.5em;">
+          <button type="submit">$_("Perform Deploy")</button>
         </form>
 
     $else:
       <p>$_("No PRs in testing set.")</p>
 
   $if dev_merged_status:
-      <h2> $_("Staged") </h2>
+      <h2> $_("Last Build Result") </h2>
       <pre>$dev_merged_status.git_status</pre>
 
       <br>
diff --git a/scripts/make-integration-branch.sh b/scripts/make-integration-branch.sh
