fix: Git discarding null properties #25068

NilanshBansal · 2023-07-04T12:14:01Z

Description

The git discard flow only discards the properties which were set in the last commit. If the properties are not present in the application json in the last commit and the user changes these properties to some value and after that choose to discard, the changes are not overwritten to null values.

This happens because git uses a function copyNestedNonNullProperties to overwrite the values to the last commit ones which ignores the properties if those are null.

This PR explicitly sets the null properties in the target json, so that when the user discards the initially set null properties, these get discarded.

PR fixes following issue(s)

Fixes #24920

Media

A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video

Type of change

Please delete options that are not relevant.

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

Testing

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration.
Delete anything that is not relevant

Manual
Jest
Cypress

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

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
PR is being merged under a feature flag

QA activity:

Speedbreak features have been covered
Test plan covers all impacted features and areas of interest
Test plan has been peer reviewed by project stakeholders and other QA members
Manually tested functionality on DP
We had an implementation alignment call with stakeholders post QA Round 2
Cypress test cases have been added and approved by SDET/manual QA
Added Test Plan Approved label after Cypress tests were reviewed
Added Test Plan Approved label after JUnit tests were reviewed

NilanshBansal · 2023-07-04T12:57:52Z

/build-deploy-preview

github-actions · 2023-07-04T12:58:30Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/5454983633.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 25068.
recreate: .

github-actions · 2023-07-04T13:26:09Z

Deploy-Preview-URL: https://ce-25068.dp.appsmith.com

github-actions · 2023-07-04T13:30:59Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5454629105.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

cypress/e2e/Regression/ClientSide/Git/GitDiscardChange/DiscardChanges_spec.js
cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad1_Spec.ts

To know the list of identified flaky tests - Refer here

AnaghHegde

I still don’t see the test cases for this scenario

github-actions · 2023-07-04T14:01:57Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5454957886.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

cypress/e2e/Regression/Apps/MongoDBShoppingCart_spec.ts
cypress/e2e/Regression/ClientSide/Git/GitDiscardChange/DiscardChanges_spec.js
cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad1_Spec.ts

To know the list of identified flaky tests - Refer here

github-actions · 2023-07-04T15:00:49Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5454957886.
Commit: ``.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

…sue-24920/git-discard-null-properties

NilanshBansal · 2023-07-04T17:43:14Z

/ok-to-test

github-actions · 2023-07-04T17:43:50Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5457305023.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 25068.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=25068&runId=5457305023_1

NilanshBansal · 2023-07-04T17:56:41Z

Here is the screen recording of the fixed flow on DP

Screen.Recording.2023-07-04.at.11.24.18.PM.mov

cc: @Parthvi12 @ginilpg

abhvsn

Added very minor comment.

...-server/src/test/java/com/appsmith/server/solutions/ImportExportApplicationServiceTests.java

NilanshBansal · 2023-07-04T18:47:05Z

/ok-to-test

github-actions · 2023-07-04T18:47:27Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5457669634.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 25068.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=25068&runId=5457669634_1

github-actions · 2023-07-04T18:53:53Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5457305023.
Commit: ``.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

Requested changes done

github-actions · 2023-07-04T19:55:13Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5457669634.
Commit: ``.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

## Description > The git discard flow only discards the properties which were set in the last commit. If the properties are not present in the application json in the last commit and the user changes these properties to some value and after that choose to discard, the changes are not overwritten to null values. > This happens because git uses a function `copyNestedNonNullProperties` to overwrite the values to the last commit ones which ignores the properties if those are null. > This PR explicitly sets the null properties in the target json, so that when the user discards the initially set null properties, these get discarded. #### PR fixes following issue(s) Fixes #24920 #### Media > A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video > > #### Type of change > Please delete options that are not relevant. - Bug fix (non-breaking change which fixes an issue) ## Testing > #### How Has This Been Tested? > Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration. > Delete anything that is not relevant - [ ] Manual - [ ] Jest - [ ] Cypress > > #### Test Plan > Add Testsmith test cases links that relate to this PR > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed (cherry picked from commit 081d417)

NilanshBansal added 25 commits May 12, 2023 15:52

Merge branch 'release' of github.com:appsmithorg/appsmith into release

54877ee

Merge branch 'release' of github.com:appsmithorg/appsmith into release

5fa31b0

Merge branch 'release' of github.com:appsmithorg/appsmith into release

9b34c1c

Merge branch 'release' of github.com:appsmithorg/appsmith into release

5c98a88

Merge branch 'release' of github.com:appsmithorg/appsmith into release

de8e537

Merge branch 'release' of github.com:appsmithorg/appsmith into release

6161384

Merge branch 'release' of github.com:appsmithorg/appsmith into release

7b641e1

Merge branch 'release' of github.com:appsmithorg/appsmith into release

09cccf9

Merge branch 'release' of github.com:appsmithorg/appsmith into release

c28149c

Merge branch 'release' of github.com:appsmithorg/appsmith into release

3123ad6

Merge branch 'release' of github.com:appsmithorg/appsmith into release

b8ed623

Merge branch 'release' of github.com:appsmithorg/appsmith into release

e767f32

Merge branch 'release' of github.com:appsmithorg/appsmith into release

0f40417

Merge branch 'release' of github.com:appsmithorg/appsmith into release

b44ec5e

Merge branch 'release' of github.com:appsmithorg/appsmith into release

c4a7345

Merge branch 'release' of github.com:appsmithorg/appsmith into release

560aa3a

Merge branch 'release' of github.com:appsmithorg/appsmith into release

705619a

Merge branch 'release' of github.com:appsmithorg/appsmith into release

a03b4d5

Merge branch 'release' of github.com:appsmithorg/appsmith into release

e60a631

Merge branch 'release' of github.com:appsmithorg/appsmith into release

5153b16

Merge branch 'release' of github.com:appsmithorg/appsmith into release

653cb91

Merge branch 'release' of github.com:appsmithorg/appsmith into release

6a3d482

Merge branch 'release' of github.com:appsmithorg/appsmith into release

449d8d6

Merge branch 'release' of github.com:appsmithorg/appsmith into release

441b764

fix: discarding git app null properties

859fbc4

NilanshBansal requested review from AnaghHegde and nayan-rafiq as code owners July 4, 2023 12:14

github-actions bot added App Navigation Issues related to the topbar navigation and configuring it Widgets Product This label groups issues related to widgets Bug Something isn't working labels Jul 4, 2023

NilanshBansal enabled auto-merge (squash) July 4, 2023 12:56

NilanshBansal disabled auto-merge July 4, 2023 12:57

AnaghHegde previously requested changes Jul 4, 2023

View reviewed changes

NilanshBansal added 3 commits July 4, 2023 23:06

added tests

0573cc8

Merge branch 'release' of github.com:appsmithorg/appsmith into fix/is…

aabb729

…sue-24920/git-discard-null-properties

indent

1286170

NilanshBansal requested a review from AnaghHegde July 4, 2023 17:40

abhvsn reviewed Jul 4, 2023

View reviewed changes

...-server/src/test/java/com/appsmith/server/solutions/ImportExportApplicationServiceTests.java Show resolved Hide resolved

asserted before also

0905a58

NilanshBansal requested a review from abhvsn July 4, 2023 18:38

abhvsn approved these changes Jul 4, 2023

View reviewed changes

NilanshBansal enabled auto-merge (squash) July 4, 2023 18:48

NilanshBansal disabled auto-merge July 4, 2023 19:39

NilanshBansal merged commit 081d417 into release Jul 4, 2023

NilanshBansal deleted the fix/issue-24920/git-discard-null-properties branch July 4, 2023 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Git discarding null properties #25068

fix: Git discarding null properties #25068

NilanshBansal commented Jul 4, 2023

NilanshBansal commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

AnaghHegde left a comment

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

NilanshBansal commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

NilanshBansal commented Jul 4, 2023 •

edited

Loading

abhvsn left a comment

NilanshBansal commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

fix: Git discarding null properties #25068

fix: Git discarding null properties #25068

Conversation

NilanshBansal commented Jul 4, 2023

Description

PR fixes following issue(s)

Media

Type of change

Testing

How Has This Been Tested?

Test Plan

Issues raised during DP testing

Checklist:

Dev activity

QA activity:

NilanshBansal commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

AnaghHegde left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

NilanshBansal commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

NilanshBansal commented Jul 4, 2023 • edited Loading

abhvsn left a comment

Choose a reason for hiding this comment

NilanshBansal commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

github-actions bot commented Jul 4, 2023

NilanshBansal commented Jul 4, 2023 •

edited

Loading