-
-
Notifications
You must be signed in to change notification settings - Fork 5
SF-3666 Fix empty translationScriptureRanges for last completed build #3620
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3620 +/- ##
=======================================
Coverage 82.82% 82.82%
=======================================
Files 610 610
Lines 37414 37420 +6
Branches 6152 6151 -1
=======================================
+ Hits 30987 30992 +5
+ Misses 5494 5481 -13
- Partials 933 947 +14 ☔ View full report in Codecov by Sentry. |
RaymondLuong3
left a comment
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 code looks good to me. Let's get this to testers.
@RaymondLuong3 reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pmachapman).
src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2574 at r1 (raw file):
// Add the training scripture ranges if (buildDto.AdditionalInfo.TrainingScriptureRanges.Count == 0)
So this looks like it was the issue and we should not have been clearing and setting the Scripture ranges to the value stored on the project if it had previously been set via the event metrics. Did I read that correctly?
Code quote:
if (buildDto.AdditionalInfo.TrainingScriptureRanges.Count == 0)src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2740 at r1 (raw file):
/// <param name="preTranslate">If <c>true</c>, return NMT builds only; otherwise, return SMT builds.</param> /// <returns>The Serval build DTO.</returns> private async Task<ServalBuildDto> MapDtoAsync(
This makes a lot of sense to have this as a method on its own. Thanks for cleaning up the instances where the build DTO is retrieved and modified.
Code quote:
private async Task<ServalBuildDto> MapDtoAsync(
pmachapman
left a comment
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.
@pmachapman made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2574 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
So this looks like it was the issue and we should not have been clearing and setting the Scripture ranges to the value stored on the project if it had previously been set via the event metrics. Did I read that correctly?
Perhaps? I do remember you mentioning these values being wrong, and with this PR they should be now correct.
This PR:
This change is