Skip to content

LT-22654: Require Webonary credentials before enabling Submit - #1112

Open
thejambi wants to merge 5 commits into
mainfrom
LT-22654
Open

LT-22654: Require Webonary credentials before enabling Submit#1112
thejambi wants to merge 5 commits into
mainfrom
LT-22654

Conversation

@thejambi

@thejambi thejambi commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Quick Summary

In Upload to Webonary dialog, Submit stayed enabled with Site name, Username, or Password blank. Clicking Submit started an upload that the controller refused immediately, but the refusal only reached the upload log, and the progress bar was left in marquee style, so the dialog appeared to upload forever.

Gate Submit on all three fields, and complete a refused upload so the dialog stops showing one as running when a publication or configuration is missing.

CI-ready checklist

  • Commit messages follow .github/commit-guidelines.md.
  • Builds & tests pass locally (or I've run the CI-style build via build.ps1, test.ps1, or MSBuild).
  • If this is core-developer AI-assisted work, I followed Docs/workflows/ai-pr-workflow.md and ran pr-preflight or the equivalent branch-readiness review before requesting review.
  • For any Src/** folders touched, corresponding AGENTS.md files are updated or explicitly confirmed still accurate.
  • I have considered all comments from an AI code reviewer (such as [Devin]https://app.devin.ai/review/sillsdev/FieldWorks/pull/####)

Notes for reviewers (optional)


This change is Reviewable

In Upload to Webonary dialog, Submit stayed enabled with Site name, Username, or
Password blank. Clicking Submit started an upload that the controller refused
immediately, but the refusal only reached the upload log, and the progress bar
was left in marquee style, so the dialog appeared to upload forever.

Gate Submit on all three fields, and complete a refused upload so the
dialog stops showing one as running when a publication or configuration
is missing.
@codecov-commenter

codecov-commenter commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.43%. Comparing base (b8f5463) to head (cfdfa00).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
Src/xWorks/UploadToWebonaryDlg.cs 90.90% 1 Missing ⚠️
Src/xWorks/WebonaryUploadLog.cs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1112      +/-   ##
==========================================
+ Coverage   38.35%   38.43%   +0.08%     
==========================================
  Files        1507     1507              
  Lines      350617   350661      +44     
  Branches    40298    40307       +9     
==========================================
+ Hits       134471   134787     +316     
+ Misses     186916   186641     -275     
- Partials    29230    29233       +3     
Files with missing lines Coverage Δ
Src/xWorks/UploadToWebonaryController.cs 67.63% <100.00%> (+0.71%) ⬆️
Src/xWorks/UploadToWebonaryDlg.Designer.cs 99.44% <100.00%> (+99.44%) ⬆️
Src/xWorks/UploadToWebonaryDlg.cs 8.66% <90.90%> (+8.24%) ⬆️
Src/xWorks/WebonaryUploadLog.cs 82.00% <83.33%> (+66.44%) ⬆️

... and 9 files with indirect coverage changes

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

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   12m 57s ⏱️ + 2m 16s
5 871 tests +9  5 790 ✅ +9  81 💤 ±0  0 ❌ ±0 
5 880 runs  +9  5 799 ✅ +9  81 💤 ±0  0 ❌ ±0 

Results for commit cfdfa00. ± Comparison against base commit 614b96e.

♻️ This comment has been updated with latest results.

Zachary Burnham added 2 commits August 27, 2026 15:36
UploadCompleted flushes the upload log before the report reads it. The
flush waited on the background write tasks unguarded, so a failed write
raised AggregateException from the finally block of UploadToWebonary,
where it would replace the exception already propagating and escape the
upload entirely. The queued tasks are never cleared, so one failed write
resurfaced on every later flush in the session.

Make the flush best effort. A caller flushing the log cannot recover
from a failed write, and losing log content must not lose what it
describes.
A credential of only whitespace passed the upload guards, and a padded
site name reached Webonary as a URL path segment and as a directory
name, both of which carried the padding along.

Widen the site name, user name, and password guards to reject blank
values, and trim the site name when it is normalized. A null or blank
site name now normalizes to empty, which the guards report as missing.
Publication and configuration stay on an emptiness test, since they are
selections rather than typed entries.
@thejambi
thejambi marked this pull request as ready for review August 27, 2026 20:29
Comment thread Src/xWorks/UploadToWebonaryDlg.cs Outdated
{
publishButton.Enabled = !string.IsNullOrWhiteSpace(webonarySiteNameTextbox.Text)
&& !string.IsNullOrWhiteSpace(webonaryUsernameTextbox.Text)
&& !string.IsNullOrWhiteSpace(webonaryPasswordTextbox.Text);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would validate this assumption that a webonary password can't be just whitespace. Eg if my password is " " then I can no longer login. If Webonary prevents you from using a password which is empty then that's maybe ok, but that could change in the future and should probably just be handled by the case where auth fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll fix this for password and username for same reason.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

Comment thread Src/xWorks/UploadToWebonaryDlg.cs Outdated
{
publishButton.Enabled = !string.IsNullOrWhiteSpace(webonarySiteNameTextbox.Text)
&& !string.IsNullOrWhiteSpace(webonaryUsernameTextbox.Text)
&& !string.IsNullOrWhiteSpace(webonaryPasswordTextbox.Text);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll fix this for password and username for same reason.

The user name and password guards rejected a value of only whitespace.
Webonary may not do the same, so we need to allow whitespace values.

Treat only an empty credential as absent. The site name keeps rejecting
whitespace, since it is normalized into a URL path segment and a
directory name rather than passed through.
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Comment hygiene (advisory)

No comment-style violations in the lines this branch adds since origin/main.

Comment thread Src/xWorks/UploadToWebonaryDlg.cs Outdated
{
publishButton.Enabled = !string.IsNullOrWhiteSpace(webonarySiteNameTextbox.Text)
&& !string.IsNullOrWhiteSpace(webonaryUsernameTextbox.Text)
&& !string.IsNullOrWhiteSpace(webonaryPasswordTextbox.Text);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants