Skip to content

SF-3808 Stop using the webhook#3920

Open
pmachapman wants to merge 6 commits into
masterfrom
fix/SF-3808
Open

SF-3808 Stop using the webhook#3920
pmachapman wants to merge 6 commits into
masterfrom
fix/SF-3808

Conversation

@pmachapman

@pmachapman pmachapman commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

This PR replaces the webhook with a background service that polls the new GetNextFinishedBuildAsync endpoint in Serval 1.18.0. I am marking this PR as do not merge, as this endpoint is currently only on Serval QA.


This change is Reviewable

@pmachapman pmachapman added will require testing PR should not be merged until testers confirm testing is complete do not merge See PR description and/or comments for explanation labels Jun 1, 2026
Comment thread src/SIL.XForge.Scripture/Services/MachineBackgroundService.cs Dismissed
@pmachapman pmachapman temporarily deployed to screenshot_diff June 1, 2026 22:50 — with GitHub Actions Inactive
@codecov

codecov Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.89340% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.90%. Comparing base (badab4f) to head (dab4dd9).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...rc/SIL.XForge/ExceptionLogging/ExceptionHandler.cs 16.66% 5 Missing ⚠️
src/SIL.XForge/Services/EmailService.cs 0.00% 4 Missing ⚠️
...rge.Scripture/Services/MachineBackgroundService.cs 97.01% 2 Missing ⚠️
...ataAccess/DataAccessServiceCollectionExtensions.cs 75.00% 2 Missing ⚠️
...taAccess/DataAccessApplicationBuilderExtensions.cs 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3920      +/-   ##
==========================================
+ Coverage   80.88%   80.90%   +0.01%     
==========================================
  Files         634      636       +2     
  Lines       41017    41065      +48     
  Branches     6654     6672      +18     
==========================================
+ Hits        33177    33222      +45     
+ Misses       6802     6792      -10     
- Partials     1038     1051      +13     

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

@pmachapman pmachapman marked this pull request as draft June 1, 2026 22:55
@pmachapman pmachapman temporarily deployed to screenshot_diff June 1, 2026 23:45 — with GitHub Actions Inactive
@pmachapman pmachapman marked this pull request as ready for review June 2, 2026 00:00

@Nateowami Nateowami left a comment

Copy link
Copy Markdown
Collaborator

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 46 files reviewed, 1 unresolved discussion (waiting on pmachapman).


.github/workflows/release.yml line 180 at r2 (raw file):

          PARATEXT_RESOURCE_PASSWORD_BASE64: ${{secrets.paratext_resource_password_base64}}
          PARATEXT_RESOURCE_PASSWORD_HASH: ${{secrets.paratext_resource_password_hash}}
          SERVAL_CLIENT_SECRET: ${{secrets.serval_client_secret

Missing }} at end of line

@Nateowami Nateowami left a comment

Copy link
Copy Markdown
Collaborator

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 made 1 comment.
Reviewable status: 1 of 46 files reviewed, 2 unresolved discussions (waiting on pmachapman).


src/SIL.XForge/DataAccess/MemoryRepository.cs line 178 at r2 (raw file):

    public Task<T> DeleteAsync(Expression<Func<T, bool>> filter, CancellationToken _ = default)
    {
        T entity = Query().First(filter);

What's the purpose of this change?

MongoRepository calls collection.FindOneAndDeleteAsync, which says it returns The deleted document if one was deleted., implying that it doesn't throw if the document isn't matched, whereas .First(filter) is going to throw an exception. Seems like our tests will meaningfully diverge.

@pmachapman pmachapman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@pmachapman made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 48 files reviewed, 1 unresolved discussion (waiting on Nateowami).


.github/workflows/release.yml line 180 at r2 (raw file):

Previously, Nateowami wrote…

Missing }} at end of line

Done. Thank you!


src/SIL.XForge/DataAccess/MemoryRepository.cs line 178 at r2 (raw file):

Previously, Nateowami wrote…

What's the purpose of this change?

MongoRepository calls collection.FindOneAndDeleteAsync, which says it returns The deleted document if one was deleted., implying that it doesn't throw if the document isn't matched, whereas .First(filter) is going to throw an exception. Seems like our tests will meaningfully diverge.

Done. Yes I think so to - I have reverted this. I wanted it to throw if a doc was not found (i.e. to show if a test was not deleting the expected document), but I agree that the diverging from Mongo is a bridge too far. I've updated the interface and Mongo implementation to match the nullability requirements.

@pmachapman pmachapman temporarily deployed to screenshot_diff June 2, 2026 20:17 — with GitHub Actions Inactive
@RaymondLuong3 RaymondLuong3 self-assigned this Jun 2, 2026

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RaymondLuong3 reviewed 32 files and all commit messages.
Reviewable status: 32 of 48 files reviewed, 1 unresolved discussion (waiting on Nateowami).

@Nateowami Nateowami left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Nateowami resolved 1 discussion.
Reviewable status: 32 of 48 files reviewed, all discussions resolved (waiting on RaymondLuong3).

@Nateowami Nateowami dismissed their stale review June 3, 2026 03:22

Requested changes addressed

@pmachapman pmachapman force-pushed the fix/SF-3808 branch 2 times, most recently from 4510159 to a3a4f39 Compare June 3, 2026 19:20
@pmachapman pmachapman temporarily deployed to screenshot_diff June 3, 2026 19:26 — with GitHub Actions Inactive

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

@RaymondLuong3 reviewed 18 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).


test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 3904 at r4 (raw file):

            JobState.Completed,
            CancellationToken.None
        );

Does this test need to say 'ProcessBuildAsync_InvalidTranslationEngineId'? It looks like this is really testing if the translation engine ID is invalid.

Code quote:

        await env.Service.ProcessBuildAsync(
            "invalid_translation_id",
            ServalBuildId01,
            JobState.Completed,
            CancellationToken.None
        );

test/SIL.XForge.Scripture.Tests/Services/MachineBackgroundServiceTests.cs line 77 at r4 (raw file):

        // Confirm initial environment
        long siteConfigCount = await env.SiteConfigs.CountDocumentsAsync(_ => true, CancellationToken.None);

Nit: did you mean for this type to be int?

Code quote:

        long siteConfigCount = await env.SiteConfigs.CountDocumentsAsync(_ => true, CancellationToken.None);

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jun 3, 2026

@Nateowami Nateowami left a comment

Copy link
Copy Markdown
Collaborator

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: all files reviewed, 2 unresolved discussions (waiting on pmachapman and RaymondLuong3).


test/SIL.XForge.Scripture.Tests/Services/MachineBackgroundServiceTests.cs line 77 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: did you mean for this type to be int?

@RaymondLuong3 I think it's because CountDocumentsAsync returns long, and it's simpler to just stick with long than to try to cast it to int.

@pmachapman pmachapman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@pmachapman made 1 comment and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).


test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 3904 at r4 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Does this test need to say 'ProcessBuildAsync_InvalidTranslationEngineId'? It looks like this is really testing if the translation engine ID is invalid.

Done. Thank you!

@pmachapman pmachapman temporarily deployed to screenshot_diff June 4, 2026 00:23 — with GitHub Actions Inactive
@RaymondLuong3 RaymondLuong3 added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jun 5, 2026

@RaymondLuong3 RaymondLuong3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RaymondLuong3 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).


test/SIL.XForge.Scripture.Tests/Services/MachineBackgroundServiceTests.cs line 77 at r4 (raw file):

Previously, Nateowami wrote…

@RaymondLuong3 I think it's because CountDocumentsAsync returns long, and it's simpler to just stick with long than to try to cast it to int.

That makes sense. I was confused since it didn't seem like CountDocumentsAsync should return a decimal number, but makes sense this could be a big number.

@pmachapman pmachapman temporarily deployed to screenshot_diff June 15, 2026 01:21 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge See PR description and/or comments for explanation 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