Skip to content
This repository was archived by the owner on Nov 5, 2025. It is now read-only.

[FIX] Add exception to the update of required settings with empty values#367

Closed
matheusbsilva137 wants to merge 1 commit into
alphafrom
required-settings-app
Closed

[FIX] Add exception to the update of required settings with empty values#367
matheusbsilva137 wants to merge 1 commit into
alphafrom
required-settings-app

Conversation

@matheusbsilva137

Copy link
Copy Markdown
Contributor

What? ⛵

  • Add an exception when the user attempts to update a required app settings' field with an empty value. Thus, required settings will not accept empty values given in the UI.

Why? 🤔

This PR fixes this issue on Apps Engine's side.

Links 🌎

Issue - ClickUp.

PS 👀

@codecov

codecov Bot commented Jan 20, 2021

Copy link
Copy Markdown

Codecov Report

Merging #367 (d0b78dd) into alpha (fd99ca9) will decrease coverage by 2.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            alpha     #367      +/-   ##
==========================================
- Coverage   53.28%   51.25%   -2.04%     
==========================================
  Files          86       78       -8     
  Lines        2918     2796     -122     
  Branches      414      415       +1     
==========================================
- Hits         1555     1433     -122     
  Misses       1363     1363              
Impacted Files Coverage Δ
src/server/managers/AppSettingsManager.ts 100.00% <100.00%> (ø)
src/server/errors/index.ts
src/server/compiler/index.ts
src/server/storage/index.ts
src/server/managers/index.ts
src/server/bridges/index.ts
src/server/accessors/index.ts
src/server/marketplace/license/index.ts
src/server/logging/index.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd99ca9...d0b78dd. Read the comment docs.

@matheusbsilva137

Copy link
Copy Markdown
Contributor Author

In our Weekly Planning Meeting, we came up with the idea of changing this PR in order to trigger the onAppSettingsChange event before the error is thrown.
The use case we've discussed in our meeting was related to setting a default value to a field when the user assigns an empty value to it. However, there is already one default behavior of not required fields in these scenarios -- which is to assign the packageValue field's value to the setting's field.
I can't think of any other use case in which that change would be valid 🤔

Also, I feel like applying this change would bring some inconsistencies, since the onAppSettingsChange event would be triggered even when the setting hasn't been changed yet (in fact, the user may have just attempted to change the setting's value). What do you guys think? @d-gubert @lolimay @thassiov

@cuonghuunguyen

Copy link
Copy Markdown
Contributor

may relate to #390

@d-gubert

d-gubert commented Jun 2, 2021

Copy link
Copy Markdown
Member

Closing in favor of #417, which provides a less conflicting solution to the problem

@d-gubert d-gubert closed this Jun 2, 2021
@d-gubert d-gubert deleted the required-settings-app branch May 2, 2024 21:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants