From 754ecb8edc31a04df031f9cf56ea46ba8f520ff8 Mon Sep 17 00:00:00 2001
From: Gustavo Saiani <gustavo@poe.ma>
Date: Fri, 7 May 2021 11:49:40 -0300
Subject: [PATCH] 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
---
.../src/metabase/components/HistoryModal.jsx | 75 +++++++++++++------
.../collections/permissions.cy.spec.js | 20 +++--
2 files changed, 66 insertions(+), 29 deletions(-)
diff --git a/frontend/src/metabase/components/HistoryModal.jsx b/frontend/src/metabase/components/HistoryModal.jsx
index e86226781b4..9224bde9deb 100644
--- a/frontend/src/metabase/components/HistoryModal.jsx
+++ b/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>
diff --git a/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js b/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js
index 6af46ef2f30..96e6ae936f4 100644
--- a/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js
+++ b/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();
+}
--
GitLab