-
Notifications
You must be signed in to change notification settings - Fork 79
Conversation
Signed-off-by: Lucas Sales <lucas.sales@zup.com.br>
Codecov Report
@@ Coverage Diff @@
## main #726 +/- ##
============================================
+ Coverage 77.48% 77.88% +0.40%
- Complexity 1978 2064 +86
============================================
Files 1192 1209 +17
Lines 16408 16854 +446
Branches 1493 1566 +73
============================================
+ Hits 12713 13126 +413
- Misses 3229 3237 +8
- Partials 466 491 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Lucas Sales <lucas.sales@zup.com.br>
ui/src/modules/Circles/Comparation/Item/CreateSegments/__tests__/Percentage.spec.tsx
Outdated
Show resolved
Hide resolved
ui/src/modules/Circles/Comparation/Item/CreateSegments/__tests__/Percentage.spec.tsx
Outdated
Show resolved
Hide resolved
ui/src/modules/Circles/Comparation/Item/CreateSegments/__tests__/Percentage.spec.tsx
Outdated
Show resolved
Hide resolved
ui/src/modules/Circles/Comparation/Item/Percentage/__tests__/CirclePercentageList.spec.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Lucas Sales <lucas.sales@zup.com.br>
ui/src/core/assets/svg/alert.svg
Outdated
<g id="Group-16" transform="translate(1098.000000, 0.000000)"> | ||
<g id="Icon/Arrow/Straight/Left-Copy-3" transform="translate(75.000000, 312.000000)"> | ||
<mask id="mask-2" fill="white"> | ||
<use xlink:href="#path-1"></use> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change path-1 to path-alert.
ui/src/core/assets/svg/alert.svg
Outdated
<mask id="mask-2" fill="white"> | ||
<use xlink:href="#path-1"></use> | ||
</mask> | ||
<use id="Icon-Alert-Triangle" fill="#FFD60A" xlink:href="#path-1"></use> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<use id="Icon-Alert-Triangle" fill="#FFD60A" xlink:href="#path-1"></use> | |
<use id="Icon-Alert-Triangle" fill="currentColor" xlink:href="#path-1"></use> |
<mask id="mask-2-alternate-down" fill="white"> | ||
<use xlink:href="#path-1-alternate-down"></use> | ||
</mask> | ||
<use id="Icon-Chevron-Down" fill="#B9B9BE" xlink:href="#path-1-alternate-down"></use> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<use id="Icon-Chevron-Down" fill="#B9B9BE" xlink:href="#path-1-alternate-down"></use> | |
<use id="Icon-Chevron-Down" fill="currentColor" xlink:href="#path-1-alternate-down"></use> |
ui/src/core/assets/svg/manually.svg
Outdated
<mask id="mask-2-manually" fill="white"> | ||
<use xlink:href="#path-1-manually"></use> | ||
</mask> | ||
<use id="Icon-Edit" fill="#B9B9BE" xlink:href="#path-1-manually"></use> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<use id="Icon-Edit" fill="#B9B9BE" xlink:href="#path-1-manually"></use> | |
<use id="Icon-Edit" fill="currentColor" xlink:href="#path-1-manually"></use> |
<mask id="mask-2-percentage" fill="white"> | ||
<use xlink:href="#path-1-percentage"></use> | ||
</mask> | ||
<use id="Icon-Percent" fill="#B9B9BE" xlink:href="#path-1-percentage"></use> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<use id="Icon-Percent" fill="#B9B9BE" xlink:href="#path-1-percentage"></use> | |
<use id="Icon-Percent" fill="currentColor" xlink:href="#path-1-percentage"></use> |
ui/src/core/assets/svg/up.svg
Outdated
<mask id="mask-2-up" fill="white"> | ||
<use xlink:href="#path-1-up"></use> | ||
</mask> | ||
<use id="Icon-Chevron-Up" fill="#B9B9BE" transform="translate(4.666833, 2.666750) scale(1, -1) translate(-4.666833, -2.666750) " xlink:href="#path-1-up"></use> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<use id="Icon-Chevron-Up" fill="#B9B9BE" transform="translate(4.666833, 2.666750) scale(1, -1) translate(-4.666833, -2.666750) " xlink:href="#path-1-up"></use> | |
<use id="Icon-Chevron-Up" fill="currentColor" transform="translate(4.666833, 2.666750) scale(1, -1) translate(-4.666833, -2.666750) " xlink:href="#path-1-up"></use> |
placeholder="Value" | ||
ref={register({ | ||
required: 'Percentage is required.', | ||
validate: value => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of putting this in utils?
Signed-off-by: Lucas Sales <lucas.sales@zup.com.br>
Signed-off-by: Thalles Freitas <thalles.freitas@zup.com.br>
Signed-off-by: Lucas Sales <lucas.sales@zup.com.br>
Signed-off-by: thalles freitas <thalles.freitas@zup.com.br>
Signed-off-by: thalles freitas <thalles.freitas@zup.com.br>
Signed-off-by: Lucas Sales <lucas.sales@zup.com.br>
Signed-off-by: thalles freitas <thalles.freitas@zup.com.br>
@@ -36,6 +36,13 @@ public String getName() { | |||
return name; | |||
} | |||
|
|||
public static Circle from(KeyMetadata keyMetadata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify if argument is null/invalid and throw IllegalArgumentException
if so, or it may cause a NPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is done here with the ifPresent, it is necessary to do inside the class?
this.getCircleByPercentage(percentageMatched)
.ifPresent(keyMetadata -> matched.add(Circle.from(keyMetadata)));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of method is ok (null safe), but it's a good practice to avoid this kind of risk encapsulating this validation, because you cannot guarantee that everyone that will use this method has this concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.Done
return this.percentage; | ||
} | ||
|
||
public void setPercentage(Integer percentage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must not allow values lower than 0 and bigger than 100. This should be verified here and also throw IllegalArgumentException
if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the validation was inserted on the request dto entity, it is necessary to add here too? also the percentage could be null on csv and regular circles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that the use of this attribute will only be by request dto? If yes, ok. If not, the verification is a good practice (in case of not null).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not.Done
this.percentage = percentage; | ||
} | ||
|
||
public int sumPercentage(Integer percentageToSum) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of sum should also be verified before change the variable. Not < 0 and not > 100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
...in/kotlin/io/charlescd/moove/application/circle/request/CreateCircleWithPercentageRequest.kt
Show resolved
Hide resolved
...esources/db/changelog/includes/20201007113800_alter_table_circles_add_percentage_column.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check my comments, i approved instead of asking for changes
Signed-off-by: thalles freitas <thalles.freitas@zup.com.br>
Signed-off-by: thalles freitas <thalles.freitas@zup.com.br>
bcdecae
Signed-off-by: thalles freitas <thalles.freitas@zup.com.br>
4108ed9
Signed-off-by: Lucas Sales lucas.sales@zup.com.br