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

SF-3103 Warn user when project does not contain all draft chapters #2903

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

RaymondLuong3
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 commented Dec 11, 2024

When a book is essentially blank on the project selected to apply a draft to, our mongoDB only has the first chapter for text documents. One option is to create the additional text documents and then apply the drafts, but this appears to go against our normal approach to have users format and create books in Paratext.
This PR gives the user a warning if they are attempting to add a draft to a book where chapters from the draft are not all present.


This change is Reviewable

@RaymondLuong3 RaymondLuong3 added the will require testing PR should not be merged until testers confirm testing is complete label Dec 11, 2024
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.01%. Comparing base (f5423e5) to head (221a96d).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...draft-apply-dialog/draft-apply-dialog.component.ts 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2903   +/-   ##
=======================================
  Coverage   82.01%   82.01%           
=======================================
  Files         544      544           
  Lines       31652    31663   +11     
  Branches     5127     5124    -3     
=======================================
+ Hits        25958    25968   +10     
+ Misses       4941     4929   -12     
- Partials      753      766   +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pmachapman pmachapman self-assigned this Dec 11, 2024
@pmachapman pmachapman self-requested a review December 11, 2024 23:59
Copy link
Collaborator

@pmachapman pmachapman left a 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, just some feedback on how this works.

I created an empty book in Paratext, and this was the warning I got applying the draft:

image copy 1.png

I then applied the draft, then went to reapply it to the project and got this:

image.png

Can we have the singular/plural chapter(s) working correctly? Also, should this replace the book is empty message (or should we make the book is empty warning yellow like this?)

Finally, these warnings are only in the add to another project dialog. Should they be in the add to project dialog too (or maybe merge the two dialogs)? This may need to be a separate issue to this PR.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

@Nateowami
Copy link
Collaborator

but this appears to go against our normal approach to have users format and create books in Paratext.

That's mostly (or entirely?) because we expect the user to set up formatting ahead of time, since it isn't editable in SF.

In this case we're completely overwriting everything, so what formatting does/doesn't exist is irrelevant. Can you look into the feasibility of just creating all the book content? If we can't easily do it that's one thing, but I don't think there's any reason we shouldn't if we can. Unless there's something I'm not considering.

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3103-target-project-blank branch from 9cbafe7 to 852fda1 Compare December 12, 2024 23:48
Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Here is what the dialog should look like when there are missing chapters. The error text in the message box should appear.
Draft Chapters Missing.png

Should they be in the add to project dialog too (or maybe merge the two dialogs)? This may need to be a separate issue to this PR.

Good point. I am willing to reuse the apply to alternate project dialog for this. But maybe @josephmyers has thoughts on that.

That's mostly (or entirely?) because we expect the user to set up formatting ahead of time, since it isn't editable in SF

That is a good point. I looked into creating text docs while applying a draft and I think this is not a good idea. We would have to guess what permissions to set for these chapters. We could probably just use the book level permission on each chapter, but I feel kind of icky making that decision.

Reviewable status: 2 of 7 files reviewed, all discussions resolved (waiting on @pmachapman)

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

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

I'm always down for UX consistency and code reuse. I like the idea of using the "add to any project" dialog across the board, and when they click "add to project" it would autofill the project field with their current project. We would need to reassess and potentially touch up some of the text (e.g. dialog title) to account for this.

Reviewable status: 2 of 7 files reviewed, all discussions resolved (waiting on @pmachapman)

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3103-target-project-blank branch from 852fda1 to 97137f3 Compare December 16, 2024 16:55
Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

I opened up SF-3126 to track this issue.

Can we have the singular/plural chapter(s) working correctly? Also, should this replace the book is empty message

I have updated the PR to handle singular and plural. Note that this is not perfect because many languages have multiple forms of a noun for different numbers such as 0, 1, 2, few, many. But this is a start.

Reviewable status: 2 of 8 files reviewed, all discussions resolved (waiting on @pmachapman)

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Here is what the dialog should look like when there are missing chapters. The error text in the message box should appear.

I don't get that message - I still get the blue "{book} in {project} is empty" message reported above.

My SFM for Joshua is simply:

\id JOS - Paipera Tapu Back Translation


I can share this project with you, if you like?

I have updated the PR to handle singular and plural. Note that this is not perfect because many languages have multiple forms of a noun for different numbers such as 0, 1, 2, few, many. But this is a start.

Cool! This maybe useful if you think 0/1/2/few/many will affect our currently supported languages? https://jsverse.github.io/transloco/docs/plugins/message-format. If 1 or plural is all we need for now, I am happy with how you have done it.

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 401 at r3 (raw file):

   * Possible values include 'zero', 'one', 'two', 'few', 'many', and 'other'.
   */
  getPluralRule(number: number): string {

NIT: If you change the return type to Intl.LDMLPluralRule, it will enforce type checking on any comparisons.

Code quote:

string

Copy link
Collaborator Author

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Hmm, regarding the blue message, I wonder if the draft you generated has multiple chapters. If the draft only contains a single chapter, then it would understand that as all chapters are available to in Paipera Tapu Back Translation. Or there might be something else going on. Yes, please add me to that project so I can investigate

This maybe useful if you think 0/1/2/few/many will affect our currently supported languages?

That is neat. I think supporting that right now is not worth the effort, but good to know in the future!

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/xforge-common/i18n.service.ts line 401 at r3 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: If you change the return type to Intl.LDMLPluralRule, it will enforce type checking on any comparisons.

Nice! Thanks.

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@RaymondLuong3 Were you able to recreate the blue "{book} in {project} is empty" message with the PTBT project I added you to?

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

@RaymondLuong3
Copy link
Collaborator Author

@pmachapman Thanks for the reminder. Let me look at that again

@RaymondLuong3
Copy link
Collaborator Author

Ok, I see that if I try to generate a draft of Joshua from and train on books from the PT1868 resource, I don't see Joshua available as a draft book option... Maybe there was an update to serval so that empty source books do not get a draft. I suspect that the issue you are seeing is because Joshua cannot be drafted because there is no content if using PT1868 as a source. If you tried generating a draft with a different source and add to the empty Joshua book in PT1868, I wonder what you would see.

@RaymondLuong3
Copy link
Collaborator Author

@pmachapman I figured it out. You most likely created a draft using the PTBT project as a target project. Since PTBT only has the empty 1st chapter in Joshua, when you try to apply the draft to PTBT, the dialog gets notified that there is only 1 chapter in Joshua because the original target project has 1 chapter. It seems that we just discovered a related issue. The original issue I was trying to fix was if the alternate target project had only an empty book. I think I can update this PR to handle both cases of the drafting target and the alternate target having an empty book.

@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3103-target-project-blank branch from 497f674 to 90c2506 Compare January 14, 2025 17:04
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts line 140 at r5 (raw file):

          setTimeout(() => {
            // give the project select component a whole cycle to load the projects
            this.projectSelect.value = this.data.paratextId;

Hmmm I don't like when we have to resort to setTimeout, but I can't find any other way to do it short of rearchitecting the project select control.

Code quote:

          setTimeout(() => {
            // give the project select component a whole cycle to load the projects
            this.projectSelect.value = this.data.paratextId;

@pmachapman pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jan 14, 2025
@RaymondLuong3 RaymondLuong3 force-pushed the task/sf-3103-target-project-blank branch from 90c2506 to a1bd6e6 Compare January 17, 2025 02:22
@RaymondLuong3
Copy link
Collaborator Author

Previously I included a commit to merge the apply to a different project dialog with the add to project method. That is being worked in SF-3126. I've removed that commit

@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jan 17, 2025
@Nateowami Nateowami force-pushed the task/sf-3103-target-project-blank branch from a1bd6e6 to 221a96d Compare January 17, 2025 15:34
@RaymondLuong3 RaymondLuong3 merged commit 762d684 into master Jan 17, 2025
14 of 15 checks passed
@RaymondLuong3 RaymondLuong3 deleted the task/sf-3103-target-project-blank branch January 17, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants