-
-
Notifications
You must be signed in to change notification settings - Fork 5
SF-3669 Fix chapter select is blank on draft formatting page #3649
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3649 +/- ##
==========================================
- Coverage 83.29% 83.29% -0.01%
==========================================
Files 611 611
Lines 37646 37645 -1
Branches 6182 6181 -1
==========================================
- Hits 31358 31357 -1
- Misses 5332 5345 +13
+ Partials 956 943 -13 ☔ View full report in Codecov by Sentry. |
Nateowami
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.
@Nateowami made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 269 at r1 (raw file):
.pipe(quietTakeUntilDestroyed(this.destroyRef)) .subscribe(async ([source, bookNum]) => { if (this.translateSource == null) {
@RaymondLuong3 The failure is saying you could write:
this.translateSource ??= (await this.projectService.getProfile(source.draftingSources[0].projectRef)).data;409599d to
2dd90cd
Compare
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 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3).
Nateowami
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.
@Nateowami reviewed all commit messages and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 85 at r2 (raw file):
private draftSources$: Observable<DraftSourcesAsArrays> = this.draftSourcesService.getDraftProjectSources(); private _bookNum: number = 1; private bookNum$ = new Subject<number>();
_bookNum and bookNum$ are redundant and make it easy for future changes to fail to keep them in sync with each other.
Instead, bookNum$ should be a BehaviorSubject, and the getting for bookNum should return this.bookNum$.getValue(). _bookNum can then be eliminated.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 122 at r2 (raw file):
get textDocId(): TextDocId | undefined { if (this.projectId == null || this.chapterNum == null) return undefined; return new TextDocId(this.projectId, this.bookNum, this.chapterNum);
Looking at this method it seems slightly odd that bookNum isn't checked. That's because it's never null, because it gets initialized to 1. I can't think of a valid reason why they wouldn't be handled the same. Having a default book makes me concerned that we'll actually try to load that book somewhere in this component.
Later in this file I see a non-null assertion on this.textDocId. It seems like this component is kind of inconsistent about whether there's a "not initialized yet" state.
2dd90cd to
79021d6
Compare
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.
@RaymondLuong3 made 2 comments.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @Nateowami and @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 85 at r2 (raw file):
Previously, Nateowami wrote…
_bookNumandbookNum$are redundant and make it easy for future changes to fail to keep them in sync with each other.Instead,
bookNum$should be aBehaviorSubject, and the getting forbookNumshould returnthis.bookNum$.getValue()._bookNumcan then be eliminated.
Done
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 122 at r2 (raw file):
Previously, Nateowami wrote…
Looking at this method it seems slightly odd that
bookNumisn't checked. That's because it's never null, because it gets initialized to 1. I can't think of a valid reason why they wouldn't be handled the same. Having a default book makes me concerned that we'll actually try to load that book somewhere in this component.Later in this file I see a non-null assertion on
this.textDocId. It seems like this component is kind of inconsistent about whether there's a "not initialized yet" state.
I see what you mean. I have updated this component so that if the textDocId was ever null then the component will gracefully handle it
Nateowami
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.
@Nateowami reviewed 1 file and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 85 at r2 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Done
Thanks. Looks good.
Previously we relied on the target text info to determine the number of chapters with drafts. However, this is not accurate since the number of chapters is dependent on the translation source. This change updates the formatting options component to use the number of chapters in the translation source in calculating the number of drafted chapters.
This change is