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

[FIX] Update getValueById for properly getting value for BOOLEAN and STRING Setting#426

Merged
d-gubert merged 2 commits into
RocketChat:alphafrom
Shailesh351:sb_fix_get_setting_value_by_id
Jun 16, 2021
Merged

[FIX] Update getValueById for properly getting value for BOOLEAN and STRING Setting#426
d-gubert merged 2 commits into
RocketChat:alphafrom
Shailesh351:sb_fix_get_setting_value_by_id

Conversation

@Shailesh351

@Shailesh351 Shailesh351 commented Jun 9, 2021

Copy link
Copy Markdown
Contributor

Fixes #425

What? ⛵

  • Updated ISettingRead getValueById
  • Updated IServerSettingRead getValueById

Why?

Earlier Behaviour

public async getValueById(id: string): Promise<any> {
        const set = await this.settingBridge.getOneById(id, this.appId);

        if (typeof set === 'undefined') {
            throw new Error(`No Server Setting found, or it is unaccessible, by the id of "${id}".`);
        }

        return set.value || set.packageValue;
    }

The issue is with the BOOLEAN settings. When packageValue (default value) is set to true and value (current value) if false,
It returns true because of OR ||.

Similar issue when setting value is '' for STRING Setting.

Behaviour after this PR

Properly returns BOOLEAN and STRING setting

@CLAassistant

CLAassistant commented Jun 9, 2021

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Jun 9, 2021

Copy link
Copy Markdown

Codecov Report

Merging #426 (e8fa629) into alpha (9502795) will decrease coverage by 0.53%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            alpha     #426      +/-   ##
==========================================
- Coverage   49.26%   48.73%   -0.54%     
==========================================
  Files         110      102       -8     
  Lines        3410     3240     -170     
  Branches      478      476       -2     
==========================================
- Hits         1680     1579     -101     
+ Misses       1730     1661      -69     
Impacted Files Coverage Δ
src/server/accessors/ServerSettingRead.ts 100.00% <100.00%> (ø)
src/server/accessors/SettingRead.ts 100.00% <100.00%> (ø)
...ver/permissions/checkers/AppDetailChangesBridge.ts 0.00% <0.00%> (-50.00%) ⬇️
src/server/logging/index.ts
src/server/managers/index.ts
src/server/storage/index.ts
src/server/compiler/index.ts
src/server/accessors/index.ts
src/server/bridges/index.ts
src/server/errors/index.ts
... and 17 more

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 9502795...e8fa629. Read the comment docs.

@Shailesh351 Shailesh351 changed the title Fix IServerSettingRead getValueById for properly getting value for BOOLEAN and STRING Setting [FIX] Update getValueById for properly getting value for BOOLEAN and STRING Setting Jun 13, 2021

@d-gubert d-gubert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your contribution! :D

@d-gubert d-gubert merged commit bceda7d into RocketChat:alpha Jun 16, 2021
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.

[BUG] IServerSettingRead getValueById fn not working as expected for BOOLEAN/STRING Setting

3 participants