-
-
Notifications
You must be signed in to change notification settings - Fork 5
SF-3601 Navigate to the first chapter of a draft when formatting draft #3532
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 #3532 +/- ##
==========================================
- Coverage 82.92% 82.91% -0.01%
==========================================
Files 605 605
Lines 36900 36898 -2
Branches 6026 6025 -1
==========================================
- Hits 30598 30594 -4
- Misses 5389 5392 +3
+ Partials 913 912 -1 ☔ View full report in Codecov by Sentry. |
707ad39 to
70af2f3
Compare
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 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.ts line 231 at r1 (raw file):
switchMap(({ targetOps, draftOps }) => { // Look for verses that contain text. If these are present, this is a non-empty draft if (this.draftHandlingService.opsHaveContent(draftOps)) {
I think the code you removed is better, as it ensures that there is verse content in the draft. opsHaveContent will return true for even empty drafts from Serval, as the rem op or id op at the top of the document will return true.
From what I can tell, opsHaveContent isn't used anywhere else in Scripture Forge? (maybe it should be removed or updated?)
Code quote:
if (this.draftHandlingService.opsHaveContent(draftOps)) {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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.ts line 231 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
I think the code you removed is better, as it ensures that there is verse content in the draft.
opsHaveContentwill return true for even empty drafts from Serval, as theremop oridop at the top of the document will returntrue.From what I can tell,
opsHaveContentisn't used anywhere else in Scripture Forge? (maybe it should be removed or updated?)
I wrestled with this for a while. It seems like the previous behaviour would hide the chapter from showing on the generated draft tab when there is no verse content at all. I think that was too strict a condition. Since this is updated to only fetch drafts if we have revisions saved in Mongo, we can safely assume that if we get to this point, the draft is valid. At a minimum the draft of the first chapter will have content, whether it be remarks, book titles, descriptions. This makes it possible to show the book if there is no verse content, but that is reasonable given the changes.
What this check does is it will cause any chapters in a book with drafts generated with no draft content (no pretranslations at all) to be hidden and the message to show on the generated draft tab. But it will allow the first chapter (which typically has some titles and descriptions pre-translated) to be visible. This helps drafts that do not have verse content in the first chapter actually appear when the user navigates from the draft in the history list.
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.ts line 231 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I wrestled with this for a while. It seems like the previous behaviour would hide the chapter from showing on the generated draft tab when there is no verse content at all. I think that was too strict a condition. Since this is updated to only fetch drafts if we have revisions saved in Mongo, we can safely assume that if we get to this point, the draft is valid. At a minimum the draft of the first chapter will have content, whether it be remarks, book titles, descriptions. This makes it possible to show the book if there is no verse content, but that is reasonable given the changes.
What this check does is it will cause any chapters in a book with drafts generated with no draft content (no pretranslations at all) to be hidden and the message to show on the generated draft tab. But it will allow the first chapter (which typically has some titles and descriptions pre-translated) to be visible. This helps drafts that do not have verse content in the first chapter actually appear when the user navigates from the draft in the history list.
OK - no worries! Thanks for the explanation of your thinking.
36d47e5 to
5918f9b
Compare
This PR updates the behaviour on the draft formatting page so only chapters with drafts can be navigated to. Additionally, in the event that the user navigates to a chapter that when the draft cannot be accessed, the page will communicate that the chapter does not contain a draft rather than the cryptic "book does not exist" message.
This change is