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

Ratings showing fixed #1516

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Ratings showing fixed #1516

wants to merge 3 commits into from

Conversation

ZdorovenkoKyrylo
Copy link
Contributor

No description provided.

@ZdorovenkoKyrylo ZdorovenkoKyrylo requested review from a team and DmyMi July 23, 2024 09:41
@@ -43,6 +43,7 @@ public async Task<AverageRatingDto> GetByEntityIdAsync(Guid entityId)
logger.LogInformation("Getting the average rating by workshop's or provider's id started.");

var rating = (await averageRatingRepository.GetByFilter(r => r.EntityId == entityId).ConfigureAwait(false)).SingleOrDefault();
rating.Rate = (float)Math.Round(rating.Rate);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but when I see "showing" bug, I expect it to be fixed somewhere in presentation layer (not business logic) using something as simple as formatting. Could it be done there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment. As far as I understand this rounding must be working either in elastic or in database. So, I found that like the only way to implement that requirement. Maybe it will be reasonable to move somewhere this logic but I assume it won`t, because the presentation in both elastic and database seems pretty similar to changing logic to me.

Copy link
Contributor Author

@ZdorovenkoKyrylo ZdorovenkoKyrylo Jul 26, 2024

Choose a reason for hiding this comment

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

@VadymLevkovskyi I also had some changes in mapping lines, you probably saw it when reviewed. That is how I tried to fix the problem with ratings. There were no visible ratings in elastic. Then the version of the elastic was changed and I do not know what was the reason but now ratings work even without those changes in mapping. that is why PR is still draft. And maybe you have some thoughts on this issue. I do not deny I might forget something while checking that is why I am still thinking about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, can't give any suggestion why it works/or doesn`t.
Now I'm resolving this comment, and asking you to either undraft (proceed with fixing the code) or close the PR

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@@ -1,3 +1,4 @@
using AutoMapper;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this using needed?

@ZdorovenkoKyrylo ZdorovenkoKyrylo marked this pull request as ready for review August 12, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants