Skip to content

Conversation

@pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Nov 30, 2025

This PR fixes a bug on QA, blocking its release to live, so I have marked as critical path.


This change is Reviewable

@pmachapman pmachapman added critical path PRs that are on our critical path and need attention to keep things moving. Priority 2nd. will require testing PR should not be merged until testers confirm testing is complete labels Nov 30, 2025
@codecov
Copy link

codecov bot commented Nov 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.78%. Comparing base (b78d54a) to head (714a35b).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3595      +/-   ##
==========================================
- Coverage   82.78%   82.78%   -0.01%     
==========================================
  Files         610      610              
  Lines       37239    37249      +10     
  Branches     6100     6082      -18     
==========================================
+ Hits        30830    30835       +5     
- Misses       5481     5498      +17     
+ Partials      928      916      -12     

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

@marksvc marksvc self-assigned this Dec 1, 2025
@marksvc marksvc added the e2e Run e2e tests for this pull request label Dec 1, 2025
if (project.TranslateConfig.DraftConfig.CurrentScriptureRange != currentScriptureRange)
string currentScriptureRange = GetDraftedScriptureRange(lastTranslationBuild);
if (
!string.IsNullOrWhiteSpace(currentScriptureRange)
Copy link
Collaborator

Choose a reason for hiding this comment

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

📌 That's interesting. What are you thinking with regard to currentScriptureRange possibly being null or whitespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These will be older builds that did not use the scripture range format.

string currentScriptureRange = GetDraftedScriptureRange(lastTranslationBuild);
if (
!string.IsNullOrWhiteSpace(currentScriptureRange)
&& project.TranslateConfig.DraftConfig.CurrentScriptureRange != currentScriptureRange
Copy link
Collaborator

Choose a reason for hiding this comment

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

📌 Actually, how would this be possible? If ExecuteWebhookAsync happens, and we mark a build as completed and run RetrievePreTranslationStatusAsync (from ExecuteWebhookAsync), wouldn't we have updated project.TranslateConfig.DraftConfig.CurrentScriptureRange from that completed build? Are we doing this because maybe we might not have heard back from a webhook, and so here we tidy up in case we had never heard back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we doing this because maybe we might not have heard back from a webhook, and so here we tidy up in case we had never heard back?

Yes.

);

// RetrievePreTranslationStatusAsync is run via a background job, so we verify that no new job was scheduled
env.BackgroundJobClient.DidNotReceive().Create(Arg.Any<Job>(), Arg.Any<IState>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

📌 Oh, yes. Good fixing this check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@marksvc
Copy link
Collaborator

marksvc commented Dec 1, 2025

Looks like still E2E failures :(

@pmachapman pmachapman removed the e2e Run e2e tests for this pull request label Dec 2, 2025
Copy link
Collaborator Author

@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.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @marksvc)

if (project.TranslateConfig.DraftConfig.CurrentScriptureRange != currentScriptureRange)
string currentScriptureRange = GetDraftedScriptureRange(lastTranslationBuild);
if (
!string.IsNullOrWhiteSpace(currentScriptureRange)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These will be older builds that did not use the scripture range format.

string currentScriptureRange = GetDraftedScriptureRange(lastTranslationBuild);
if (
!string.IsNullOrWhiteSpace(currentScriptureRange)
&& project.TranslateConfig.DraftConfig.CurrentScriptureRange != currentScriptureRange
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we doing this because maybe we might not have heard back from a webhook, and so here we tidy up in case we had never heard back?

Yes.

);

// RetrievePreTranslationStatusAsync is run via a background job, so we verify that no new job was scheduled
env.BackgroundJobClient.DidNotReceive().Create(Arg.Any<Job>(), Arg.Any<IState>());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@pmachapman pmachapman force-pushed the fix/SF-3655 branch 2 times, most recently from 4495143 to 060ae5c Compare December 2, 2025 22:42
@pmachapman pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Dec 3, 2025
@Nateowami Nateowami added the e2e Run e2e tests for this pull request label Dec 9, 2025
@Nateowami
Copy link
Collaborator

Merging so we can finish testing on QA.

@Nateowami Nateowami merged commit 7d5fb11 into master Dec 9, 2025
27 of 28 checks passed
@Nateowami Nateowami deleted the fix/SF-3655 branch December 9, 2025 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

critical path PRs that are on our critical path and need attention to keep things moving. Priority 2nd. e2e Run e2e tests for this pull request ready to test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants