Omit revision history item from list if no `diff` (#15942)

* Omit dashboard revision history items if no description

* Skip Revert button on topmost visible revision history entry

* Test that topmost revision history entry omits Revert button

* Lint

* Use diff as criterion for omitting revision entry

* Detail comment in shouldRenderRevisionHistory

* Render revision item if either before or after in diff is populated

* Update e2e test

* Use a more durable string to find element in test

frontend/src/metabase/components/HistoryModal.jsx

@@ -24,7 +24,7 @@ export default class HistoryModal extends Component {
     onClose: PropTypes.func.isRequired,
   };
 
-  revisionDescription(revision) {
+  getRevisionDescription(revision) {
     if (revision.is_creation) {
       return t`First revision.`;
     } else if (revision.is_reversion) {
@@ -34,10 +34,27 @@ export default class HistoryModal extends Component {
     }
   }
 
+  shouldRenderRevisionEntry({ diff }) {
+    // diff may be null in "First revision"
+    // or in the earliest revision kept in store
+    if (diff === null) {
+      return true;
+    }
+
+    const { before, after } = diff;
+    return before !== null || after !== null;
+  }
+
   render() {
     const { revisions, onRevert, onClose } = this.props;
     const cellClassName = "p1 border-bottom";
 
+    // We must keep track of having skipped the Revert button
+    // because we are omitting certain revision entries,
+    // see function shouldRenderRevisionEntry
+    // They may be the very top entry so we have to use dedicated logic.
+    let hasSkippedMostRecentRevisionRevertButton = false;
+
     return (
       <ModalContent title={t`Revision history`} onClose={onClose}>
         <table className="full">
@@ -50,29 +67,39 @@ export default class HistoryModal extends Component {
             </tr>
           </thead>
           <tbody>
-            {revisions.map((revision, index) => (
-              <tr key={revision.id}>
-                <td className={cellClassName}>
-                  {formatDate(revision.timestamp)}
-                </td>
-                <td className={cellClassName}>{revision.user.common_name}</td>
-                <td className={cellClassName}>
-                  <span>{this.revisionDescription(revision)}</span>
-                </td>
-                <td className={cellClassName}>
-                  {index !== 0 && onRevert && (
-                    <ActionButton
-                      actionFn={() => onRevert(revision)}
-                      className="Button Button--small Button--danger text-uppercase"
-                      normalText={t`Revert`}
-                      activeText={t`Reverting…`}
-                      failedText={t`Revert failed`}
-                      successText={t`Reverted`}
-                    />
-                  )}
-                </td>
-              </tr>
-            ))}
+            {revisions.map((revision, index) => {
+              if (this.shouldRenderRevisionEntry(revision)) {
+                const shouldRenderRevertButton =
+                  onRevert && hasSkippedMostRecentRevisionRevertButton;
+                hasSkippedMostRecentRevisionRevertButton = true;
+
+                return (
+                  <tr key={revision.id}>
+                    <td className={cellClassName}>
+                      {formatDate(revision.timestamp)}
+                    </td>
+                    <td className={cellClassName}>
+                      {revision.user.common_name}
+                    </td>
+                    <td className={cellClassName}>
+                      <span>{this.getRevisionDescription(revision)}</span>
+                    </td>
+                    <td className={cellClassName}>
+                      {shouldRenderRevertButton && (
+                        <ActionButton
+                          actionFn={() => onRevert(revision)}
+                          className="Button Button--small Button--danger text-uppercase"
+                          normalText={t`Revert`}
+                          activeText={t`Reverting…`}
+                          failedText={t`Revert failed`}
+                          successText={t`Reverted`}
+                        />
+                      )}
+                    </td>
+                  </tr>
+                );
+              }
+            })}
           </tbody>
         </table>
       </ModalContent>

frontend/test/metabase/scenarios/collections/permissions.cy.spec.js

@@ -528,17 +528,22 @@ describe("collection permissions", () => {
         cy.signInAsAdmin();
       });
 
-      it.skip("shouldn't record history steps when there was no diff (metabase#1926)", () => {
+      it("shouldn't render revision history steps when there was no diff (metabase#1926)", () => {
         cy.signInAsAdmin();
         cy.createDashboard("foo").then(({ body }) => {
           visitAndEditDashboard(body.id);
         });
+
         // Save the dashboard without any changes made to it (TODO: we should probably disable "Save" button in the first place)
         saveDashboard();
-        // Take a look at the generated history - there shouldn't be anything other than "First revision" (dashboard created)
-        cy.icon("ellipsis").click();
-        cy.findByText("Revision history").click();
-        cy.findAllByRole("button", { name: "Revert" }).should("not.exist");
+        cy.icon("pencil").click();
+        saveDashboard();
+
+        openRevisionHistory();
+
+        cy.findByText("First revision.");
+
+        cy.findAllByText("Revert").should("not.exist");
       });
 
       it.skip("dashboard should update properly on revert (metabase#6884)", () => {
@@ -706,3 +711,8 @@ function saveDashboard() {
   clickButton("Save");
   cy.findByText("You're editing this dashboard.").should("not.exist");
 }
+
+function openRevisionHistory() {
+  cy.icon("ellipsis").click();
+  cy.findByText("Revision history").click();
+}