fix: Add migration to fix deleted themes when branch was deleted by nayan-rafiq · Pull Request #14957 · appsmithorg/appsmith

nayan-rafiq · 2022-07-01T13:24:12Z

Description

Due to a bug, customized themes set to the parent branch where deleted when user deletes git branch. The bug has been fixed already. This PR adds a DB migration that fixes the data if there is any.

Fixes #15247

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Manually

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

vercel · 2022-07-01T13:24:15Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
appsmith	✅ Ready (Inspect)	Visit Preview	Jul 26, 2022 at 10:09AM (UTC)

github-actions · 2022-07-01T13:25:10Z

Unable to find test scripts. Please add necessary tests to the PR.

github-actions · 2022-07-03T11:36:46Z

Unable to find test scripts. Please add necessary tests to the PR.

github-actions · 2022-07-03T11:36:50Z

Unable to find test scripts. Please add necessary tests to the PR.

abhvsn · 2022-07-03T16:27:05Z

+        Criteria deletedCustomThemes = Criteria.where(fieldName(QTheme.theme.id)).in(set)
+                .and(fieldName(QTheme.theme.deleted)).is(true);
+
+        mongockTemplate.updateMulti(new Query(deletedCustomThemes), update, Theme.class);


@nayan-rafiq the issue is happening when users have a custom theme with multiple branches and if any of the branches is deleted all the branches will end up in a broken state. This is happening because the same themeIds are referenced in all the branched applications, where we expect separate objects in Theme collection with separate themeIds for different branches. With the current migration, I'm not getting where are we inserting new documents in the Theme collection and updating the corresponding ids in the branched application object in DB. IIUC this migration will only revert the deleted custom themes

When user has customized the theme and created a new branch, the newly created branch have the same theme ids as parent branch (it should've been copied). Now, if user deletes the new branch, it deletes the themes as well. So, if we set deleted=false to those themes, would it not be enough? It'll be a problem if we've option to restore a deleted branch. Is that happening today?

@nayan-rafiq consider user has already created 3 branches before we fixed the issue for the custom theme app. As per our expectation we should have 3 seperate theme ids attached to these apps which will not be the case. Now consider after we run the migration there won't be any change considering all the branches are still active.
In this scenario (after we run the migration) what will happen if user tries to delete the earlier branch (out of 3 branches created earlier)? As we are not checking if the theme id is being used in some other branch, it will get deleted and we will end up in same situation again.

github-actions · 2022-07-12T16:43:56Z

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

github-actions · 2022-07-19T16:45:39Z

This PR has been closed because of inactivity.

AnaghHegde · 2022-07-21T18:12:57Z

/ok-to-test sha=1a29e4e

github-actions · 2022-07-21T18:13:35Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2713807557.
Workflow: Appsmith External Integration Test Workflow.
Commit: 1a29e4e.
PR: 14957.

AnaghHegde · 2022-07-26T10:10:15Z

/ok-to-test sha=7b12b2a

github-actions · 2022-07-26T10:10:46Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2738704052.
Workflow: Appsmith External Integration Test Workflow.
Commit: 7b12b2a.
PR: 14957.

github-actions · 2022-07-26T10:56:21Z

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2738704052.
Commit: 7b12b2a.
Results:

Click to view performance test results

	Run 1	Run 2	Run 3	Run 4	Run 5	Median	Mean	SD.Sample	SD.Population
SELECT_CATEGORY
scripting	608.58	1334.03	565.23	612.48	577.72	608.58	739.61	45.01	40.26
painting	12.85	4.52	3.77	9.52	3.45	4.52	6.82	61.14	54.69
rendering	94.76	94.96	92.97	96.27	93.62	94.76	94.52	1.35	1.21
BIND_TABLE_DATA
scripting	2354.71	2274.59	2237.86	2252.34	2278.34	2274.59	2279.57	1.98	1.77
painting	13.41	14.63	15.04	5.74	15.37	14.63	12.84	31.46	28.12
rendering	682.38	713.17	680.95	691.85	713.47	691.85	696.36	2.30	2.06
CLICK_ON_TABLE_ROW
scripting	1280.25	1478	1345.03	1301.38	1429.44	1345.03	1366.82	6.18	5.53
painting	10.46	10.53	21.01	12.63	10.79	10.79	13.08	34.56	30.89
rendering	276.53	284.03	290.17	281.77	288.46	284.03	284.19	1.91	1.71
UPDATE_POST_TITLE
scripting	2472.85	2352.83	2441.55	2425.31	2404.99	2425.31	2419.51	1.85	1.66
painting	16.26	17.66	19.54	14.84	17.5	17.5	17.16	10.20	9.09
rendering	393.71	410.49	384.33	402.92	393.71	393.71	397.03	2.52	2.25
OPEN_MODAL
scripting	784.57	824.73	745.97	775.49	823.84	784.57	790.92	4.25	3.80
painting	9.14	9.79	9.69	9.56	8.4	9.56	9.32	6.12	5.47
rendering	309.06	318.3	299.56	303.41	307.89	307.89	307.64	2.29	2.05
CLOSE_MODAL
scripting	497.4	456.69	447.03	506.96	452.83	456.69	472.18	5.89	5.27
painting	4.43	4.5	4.05	4.88	4.81	4.5	4.53	7.28	6.62
rendering	488.36	506.52	459.15	473.26	483.75	483.75	482.21	3.66	3.27
SELECT_WIDGET_MENU_OPEN
scripting	1102.55	1101.16	1125.03	1116.05	1150.15	1116.05	1118.99	1.79	1.60
painting	7.26	6.64	6.99	8.83	5.2	6.99	6.98	18.62	16.62
rendering	494.38	483.02	479.04	499.35	498.22	494.38	490.8	1.88	1.68
SELECT_WIDGET_SELECT_OPTION
scripting	156.86	161.88	185.65	150.81	152.82	156.86	161.6	8.72	7.80
painting	3.16	5.57	2.29	3.71	12.4	3.71	5.43	75.14	67.22
rendering	12.12	11.45	15.47	11.19	11.21	11.45	12.29	14.81	13.26

nayan-rafiq self-assigned this Jul 1, 2022

vercel bot deployed to Preview – appsmith July 1, 2022 13:24 View deployment

github-actions bot added the Bug Something isn't working label Jul 1, 2022

Nayan added 3 commits July 3, 2022 17:31

fix: Add migration to fix deleted themes when branch was deleted

c095a5a

fix: Added migration for the deleted themes during create branch

5c76e22

-updated mimgration number

a6a0871

nayan-rafiq force-pushed the fix/theme-deleted-when-delete-branch branch from 94ecbce to a6a0871 Compare July 3, 2022 11:32

vercel bot deployed to Preview – appsmith July 3, 2022 11:33 View deployment

nayan-rafiq marked this pull request as ready for review July 3, 2022 11:33

nayan-rafiq requested a review from abhvsn July 3, 2022 11:33

nayan-rafiq added the Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable label Jul 3, 2022

abhvsn reviewed Jul 3, 2022

View reviewed changes

nayan-rafiq requested a review from AnaghHegde July 4, 2022 16:15

github-actions bot added the Stale label Jul 12, 2022

github-actions bot closed this Jul 19, 2022

AnaghHegde reopened this Jul 20, 2022

Merge branch 'release' into fix/theme-deleted-when-delete-branch

d8427ef

vercel bot deployed to Preview – appsmith July 21, 2022 11:34 View deployment

AnaghHegde self-assigned this Jul 21, 2022

Add migration to fix the themes not copied in git connected apps

1a29e4e

vercel bot deployed to Preview – appsmith July 21, 2022 18:13 View deployment

AnaghHegde requested review from trishaanand and removed request for AnaghHegde July 21, 2022 18:14

github-actions bot removed the Stale label Jul 22, 2022

Add check for system themes

1a09e56

vercel bot deployed to Preview – appsmith July 25, 2022 10:02 View deployment

Nayan added 2 commits July 25, 2022 16:14

-added migration for the same custom theme in two branches

922e626

-select only id to improve running time

7b12b2a

AnaghHegde requested a review from mohanarpit July 26, 2022 10:09

AnaghHegde requested a review from abhvsn July 26, 2022 10:35

mohanarpit approved these changes Jul 26, 2022

View reviewed changes

sharat87 merged commit 9489484 into release Jul 27, 2022

sharat87 deleted the fix/theme-deleted-when-delete-branch branch July 27, 2022 04:43

Conversation

nayan-rafiq commented Jul 1, 2022 • edited by mohanarpit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

vercel bot commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 1, 2022

Uh oh!

github-actions bot commented Jul 3, 2022

Uh oh!

github-actions bot commented Jul 3, 2022

Uh oh!

abhvsn Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nayan-rafiq Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

abhvsn Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 12, 2022

Uh oh!

github-actions bot commented Jul 19, 2022

Uh oh!

AnaghHegde commented Jul 21, 2022

Uh oh!

github-actions bot commented Jul 21, 2022

Uh oh!

AnaghHegde commented Jul 26, 2022

Uh oh!

github-actions bot commented Jul 26, 2022

Uh oh!

github-actions bot commented Jul 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nayan-rafiq commented Jul 1, 2022 •

edited by mohanarpit

Loading

vercel bot commented Jul 1, 2022 •

edited

Loading

abhvsn Jul 3, 2022 •

edited

Loading