Skip to content

SF-3490 Show who created a draft #3350

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

Merged
merged 3 commits into from
Aug 11, 2025
Merged

SF-3490 Show who created a draft #3350

merged 3 commits into from
Aug 11, 2025

Conversation

pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Aug 6, 2025

This PR adds a line to the bottom of the build details showing who started the build and when (when this data is available).

image

@Nateowami Was this what you were expecting for this change? I didn't add it to the date subtitle, as saying "Build completed by User 123 at Aug 6 2025, 12:01 AM" didn't really make sense to me (unless you can think of better wording?)


This change is Reviewable

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

codecov bot commented Aug 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.42%. Comparing base (910b5c4) to head (f7cffb6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3350   +/-   ##
=======================================
  Coverage   82.42%   82.42%           
=======================================
  Files         607      607           
  Lines       35537    35537           
  Branches     5789     5789           
=======================================
  Hits        29291    29291           
  Misses       5369     5369           
  Partials      877      877           

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

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@pmachapman Can you update the wording of the footer to say e.g. Completed at <date>? JoEllen and both assumed it was showing the time it was requested when we wrote the issue description.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 279 at r1 (raw file):

    "format_draft": "Format draft",
    "hide_model_training_configuration": "Hide model training configuration",
    "requested_at": "Requested at {{ requestedAtDate }}.",

In English, we'd generally say something happened on a date, or at a time. I would supposed other languages may do the same. Can you rename this variable to something like requestedAtTime or requestedAtDateTime so the translators will know it's the time, and not just the date?

(Personally I also think in English it flows better without "at". For example, "Requested July 3, 2025 4:23 PM").

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.

I named it completed, but it look funny for a cancelled build. Does describing it as finished work?

image.png

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 279 at r1 (raw file):

Previously, Nateowami wrote…

In English, we'd generally say something happened on a date, or at a time. I would supposed other languages may do the same. Can you rename this variable to something like requestedAtTime or requestedAtDateTime so the translators will know it's the time, and not just the date?

(Personally I also think in English it flows better without "at". For example, "Requested July 3, 2025 4:23 PM").

Done.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

I agree; "completed" isn't the best word for failures. Finished is good.

One minor comment, but :lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 277 at r2 (raw file):

    "draft_unknown": "Unknown",
    "error_details": "Please include these details when contacting support for assistance",
    "finished_at": "Finished at {{ finishedAtTime }}",

Maybe drop the "at" here as well?

@Nateowami Nateowami added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Aug 6, 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 277 at r2 (raw file):

Previously, Nateowami wrote…

Maybe drop the "at" here as well?

Done. I first thought it looked funny, but looking again, it looks better. Thanks!

@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 Aug 11, 2025
@Nateowami Nateowami merged commit 611cfd2 into master Aug 11, 2025
15 of 16 checks passed
@Nateowami Nateowami deleted the fix/SF-3490 branch August 11, 2025 14:13
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.

2 participants