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.

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

#3693 - [Bug] 0 is removed from the Exceptional costs field #3999

Merged
merged 7 commits into from
Nov 27, 2024

Conversation

sh16011993
Copy link
Collaborator

@sh16011993 sh16011993 commented Nov 26, 2024

As a part of this PR, the following bug was fixed:

Bug: 0 is removed from the Exceptional costs field and the warning banner as shown below shows up. Likewise, for the other 3 cost fields.

image

Fix: numericTransformer on receiving the value of 0 from the database was returning it as null rather than the numeric value 0. Updated the code to fix it and return 0 instead of null.

2024-11-26.13-05-35.mp4

@sh16011993 sh16011993 self-assigned this Nov 26, 2024
@sh16011993 sh16011993 added Web portal Institution Institution Features Bug Something isn't working labels Nov 26, 2024
@sh16011993 sh16011993 changed the title #3693 - Institution: 0 is removed from the Exceptional costs field bug #3693 - Institution: 0 is removed from the Exceptional costs field [bug] Nov 26, 2024
@sh16011993 sh16011993 changed the title #3693 - Institution: 0 is removed from the Exceptional costs field [bug] #3693 - [Bug] Institution: 0 is removed from the Exceptional costs field Nov 26, 2024
@sh16011993 sh16011993 changed the title #3693 - [Bug] Institution: 0 is removed from the Exceptional costs field #3693 - [Bug] 0 is removed from the Exceptional costs field Nov 26, 2024
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Great finding. One minor comment.

@@ -14,7 +14,7 @@ export const numericTransformer: ValueTransformer = {
* @returns numeric/decimal string converted to a number.
*/
from: (value: string | null): number | null => {
if (value) {
if (value !== null && value !== undefined) {
Copy link
Collaborator

@dheepak-aot dheepak-aot Nov 26, 2024

Choose a reason for hiding this comment

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

Suggestion from my side to use the following solution as it checks for a string which can be a number or not.

from: (value: string | null): number | null => {
    if (Number.isFinite(parseFloat(value))) {
      return parseFloat(value);
    }
    return null;
  }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for suggesting it @dheepak-aot and thanks for implementing it @sh16011993.

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.07% ( 3733 / 16913 )
Methods: 10.16% ( 214 / 2107 )
Lines: 25.42% ( 3241 / 12751 )
Branches: 13.53% ( 278 / 2055 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.43% ( 583 / 891 )
Methods: 59.26% ( 64 / 108 )
Lines: 68.54% ( 464 / 677 )
Branches: 51.89% ( 55 / 106 )

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 84.39% ( 1238 / 1467 )
Methods: 84.51% ( 120 / 142 )
Lines: 85.61% ( 1053 / 1230 )
Branches: 68.42% ( 65 / 95 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 67.04% ( 5810 / 8667 )
Methods: 64.86% ( 720 / 1110 )
Lines: 70.99% ( 4561 / 6425 )
Branches: 46.73% ( 529 / 1132 )

Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Collaborator

@guru-aot guru-aot left a comment

Choose a reason for hiding this comment

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

LGTM nice work @sh16011993

@sh16011993 sh16011993 added this pull request to the merge queue Nov 27, 2024
Merged via the queue into main with commit bbbc57e Nov 27, 2024
20 checks passed
@sh16011993 sh16011993 deleted the 3693_bug_institution_0_removed_exceptional_costs branch November 27, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working E2E/Unit tests Institution Institution Features Web portal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants