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-3240 Fix crashes when a draft book has an invalid book id #3062

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Mar 5, 2025

This PR fixes crashes that occur viewing or applying a draft, when a source book's book id is the wrong case, and the draft was generated from that source.


This change is Reviewable

@pmachapman pmachapman added the will require testing PR should not be merged until testers confirm testing is complete label Mar 5, 2025
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.78%. Comparing base (2f218f7) to head (6c99906).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3062   +/-   ##
=======================================
  Coverage   82.78%   82.78%           
=======================================
  Files         563      563           
  Lines       32528    32528           
  Branches     5271     5293   +22     
=======================================
  Hits        26929    26929           
+ Misses       4822     4810   -12     
- Partials      777      789   +12     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephmyers josephmyers self-assigned this Mar 5, 2025
@josephmyers
Copy link
Collaborator

For testing, I'm going to start the draft, per steps, and let it run overnight.

@Nateowami Nateowami requested a review from Copilot March 5, 2025 15:14

Choose a reason for hiding this comment

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

PR Overview

This PR fixes crashes that occur when a draft book has an invalid book id by ensuring that the correct stylesheet for the book is used during conversion.

  • Updated the GetBookText and GetDeltaFromUsfm methods in ParatextService to pass scrText.ScrStylesheet(bookNum) instead of the book number
  • Added two new tests to validate the behavior when a variant book id is used in USFM to USX conversions

Reviewed Changes

File Description
test/SIL.XForge.Scripture.Tests/Services/ParatextServiceTests.cs Adds tests for override behavior with variant book ids in GetBookText and GetDeltaFromUsfmAsync methods
src/SIL.XForge.Scripture/Services/ParatextService.cs Updates method calls to use scrText.ScrStylesheet(bookNum) for USFM conversion

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

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.

This change makes sense, and the draft generated from KJVA can be applied
:lgtm:

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

@josephmyers josephmyers added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Mar 6, 2025
@RaymondLuong3 RaymondLuong3 merged commit 55617cc into master Mar 6, 2025
17 of 18 checks passed
@RaymondLuong3 RaymondLuong3 deleted the fix/SF-3240 branch March 6, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants