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.

Sign up for GitHub

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

Jump to bottom

fix: Filtering out empty value env variables from /admin/env API response in backend #18430

Merged
merged 5 commits into from
Dec 9, 2022
Merged

fix: Filtering out empty value env variables from /admin/env API response in backend #18430

merged 5 commits into from
Dec 9, 2022

Conversation

vivonk
Copy link
Contributor

@vivonk vivonk commented Nov 24, 2022 •
edited

Description

This PR is part of adding improvements in /admin/env API - #18423
And with that, it fixes main issue which is - #17846

When we are sending empty value env variables in /admin/env API response, it's causing incorrect state in toggle buttons (Toggle button gets enabled for empty value variables). Because of which, we were facing incorrect config update issues in Admin settings like Email. As part of fixing that, we are sending only non-empty valued env variables so that frontend interprets it correct.

Screenshot 2022-11-24 at 11 06 55

Screenshot 2022-11-24 at 11 07 04

Fixes
#18423
#17846

Type of change

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

How Has This Been Tested?

Test Plan

  • PENDING, incase required

Issues raised during DP testing

  • NA

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:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@welcome
Copy link

welcome bot commented Nov 24, 2022

Welcome to the Appsmith community! Thank you for your first pull request and making this project better. 🤗 Please make sure that you raise a review request so your code change does not go unnoticed.

@vercel
Copy link

vercel bot commented Nov 24, 2022 •
edited

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

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Dec 9, 2022 at 6:02AM (UTC)

@github-actions
Copy link

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

5 similar comments
@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@vivonk vivonk changed the title Fix: Filtering out empty value env variables from /admin/env API response in backend fix: Filtering out empty value env variables from /admin/env API response in backend Nov 25, 2022
@github-actions github-actions bot added the Bug Something isn't working label Nov 25, 2022
@github-actions
Copy link

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

2 similar comments
@github-actions
Copy link

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

@github-actions
Copy link

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

@vivonk vivonk added the Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable label Nov 29, 2022
@trishaanand
Copy link
Contributor

@vivonk : Nirmal, is this change a breaking change? Are we dependent on the FE changes to take this to release?

@vivonk
Copy link
Contributor Author

vivonk commented Nov 29, 2022

No, @trishaanand . It's a backend change only. Frontend works fine with this change.

@vivonk
Copy link
Contributor Author

vivonk commented Dec 1, 2022

/ok-to-test sha=97c6b83ae2451389b7d162b44e6a984eb9ae3ecd

@vivonk vivonk self-assigned this Dec 1, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3592291019.
Workflow: Appsmith External Integration Test Workflow.
Commit: 97c6b83ae2451389b7d162b44e6a984eb9ae3ecd.
PR: 18430.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18430&runId=3592291019_1

@vivonk
Copy link
Contributor Author

vivonk commented Dec 6, 2022

/ok-to-test sha=87797d94589ce966a06302bad7e3cdcecd995e99

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

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

@vivonk
Copy link
Contributor Author

vivonk commented Dec 8, 2022

/ok-to-test sha=430551bcdd69624bda739b9edc8672fba2a20284

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

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

@vivonk
Copy link
Contributor Author

vivonk commented Dec 9, 2022

/ok-to-test sha=60875903d9053e82aa60d1d8a0bcca99895df902

@github-actions
Copy link

github-actions bot commented Dec 9, 2022

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

@vivonk vivonk merged commit 9f0b9ee into release Dec 9, 2022
@vivonk vivonk deleted the fix/non-empty-envs-in-response branch December 9, 2022 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trishaanand trishaanand trishaanand approved these changes

@nayan-rafiq nayan-rafiq Awaiting requested review from nayan-rafiq

Assignees

@vivonk vivonk

Labels
Bug Something isn't working Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vivonk @trishaanand