Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed width dashboard migration #38247

Merged
merged 11 commits into from
Feb 1, 2024
Merged

Conversation

adam-james-v
Copy link
Contributor

@adam-james-v adam-james-v commented Jan 30, 2024

This PR addressed the first 2 items in #36358

  • (BE) Create new width setting on dashboard object with 2 options: "full" | "fixed"
  • (BE) Create migration to set all existing dashboards to width = "full"

The need was:

  • add a :width setting on dashboards
  • the allowed values here are 'fixed' or 'full'
  • for all new dashboards, the default setting shall be 'fixed' (the new default behaviour we want)
  • for all existing dashboards, the value should be set to 'full' (the existing behaviour)

3 migrations were used to achieve this:

  • the migration adding the column, whose default value is 'fixed'. This ensures all new dashboards will have the value correctly set
  • another migration to change all existing dashboard entries to have the 'full' value. This makes sense as it won't break existing behaviours for users
  • the final migration setting the width column to notNull, ensuring it's always set going forward.

A test was added to the Dashboard api test namespace to make sure these properties are met after the migration.

I've also manually tested in the REPL:

;; an existing dashboard:
(:width (t2/select-one :model/Dashboard))
;; full.

;; a newly created dashboard:
(:width (t2/select-one :model/Dashboard :id 535))
;; fixed.

The setting is exposed to change via PUT /api/dashboard/:id.

@adam-james-v adam-james-v requested review from markbastian and removed request for camsaul January 30, 2024 00:12
@adam-james-v adam-james-v self-assigned this Jan 30, 2024
@adam-james-v adam-james-v added the .Team/DashViz Dashboard and Viz team label Jan 30, 2024
3 migrations:
 - 1st adding the width column with default value of 'fixed'
 - 2nd updating all existing dashboards to have width 'full', which corresponds to what the current behaviour is (will
 be the 'old' behaviour after the fixed-width project lands).
   - The rolloback here is necessary but we don't care what happens as the column will be dropped immediately in the
   next rollback anyway
 - 3rd sets the notNullableConstraint. DefaultNull is 'full' here, just in case there's an existing dashboard whose
 width value is not yet set from the 1st migration. Don't know how that could happen, but its here in case
update-dashboard function now is aware of the :width key so those changes can end up in the transaction.

Also added a width test that asserts that the value's default is "fixed", it can be changed, eg. to "full", but cannot
be changed to other values.
@adam-james-v adam-james-v force-pushed the fixed-width-dashboard-migration branch from ab31924 to 78960bf Compare January 30, 2024 00:45
@adam-james-v adam-james-v added the no-backport Do not backport this PR to any branch label Jan 30, 2024
Copy link

replay-io bot commented Jan 30, 2024

StatusIn Progress ↗︎ 36 / 52
Commit19f01a9
Results
1 Failed
  • should work for longitude
      AssertionError: Timed out retrying after 4000ms: Unable to find an element with the text: Saved Questions. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.
      Ignored nodes: comments, <script />, <style />
      <html
        lang="en"
        translate="no"
      >
        <head>
          <meta
            charset="utf-8"
          />
          <meta
            content="IE=edge"
            http-equiv="X-UA-Compatible"
          />
          <meta
            cont...
⚠️ 3 Flaky
1578 Passed

:width key is now needed in some revision tests. As well we need a string communicating that the :width setting has
changed from 'full' to 'fixed' or vice-versa.
Copy link
Contributor

@markbastian markbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment and remarks in the last migration and then it will be g2g.

Copy link
Contributor

@markbastian markbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@adam-james-v adam-james-v merged commit ffd1873 into master Feb 1, 2024
105 checks passed
@adam-james-v adam-james-v deleted the fixed-width-dashboard-migration branch February 1, 2024 19:37
Copy link

github-actions bot commented Feb 1, 2024

@adam-james-v Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

npfitz pushed a commit that referenced this pull request Feb 5, 2024
* Migration adding 'width' to Dashboards

3 migrations:
 - 1st adding the width column with default value of 'fixed'
 - 2nd updating all existing dashboards to have width 'full', which corresponds to what the current behaviour is (will
 be the 'old' behaviour after the fixed-width project lands).
   - The rolloback here is necessary but we don't care what happens as the column will be dropped immediately in the
   next rollback anyway
 - 3rd sets the notNullableConstraint. DefaultNull is 'full' here, just in case there's an existing dashboard whose
 width value is not yet set from the 1st migration. Don't know how that could happen, but its here in case

* Dashboard PUT api endpoint accepts width changes and updates appdb

update-dashboard function now is aware of the :width key so those changes can end up in the transaction.

Also added a width test that asserts that the value's default is "fixed", it can be changed, eg. to "full", but cannot
be changed to other values.

* Add width to revision tests

* Fix dashboard revision tests.

:width key is now needed in some revision tests. As well we need a string communicating that the :width setting has
changed from 'full' to 'fixed' or vice-versa.

* Fix comments/remarks in migration to be accurate

* Attempt to fix default not working mysql/mariadb

* Set default in dashboard model

Signed-off-by: Adam James <adam.vermeer2@gmail.com>

* Revert default :width 'fixed' value.

* Explicitly add default value 'fixed' for MySQL/MariaDB

---------

Signed-off-by: Adam James <adam.vermeer2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants