Repository navigation
Codacy bug/package cleanup - #645
Conversation
Up to standards ✅🟢 Issues
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78dd6c148e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "user": { | ||
| "email_address": "estateuser@stagingestate1.co.uk", | ||
| "password": "123456", | ||
| "password": "", |
There was a problem hiding this comment.
On a fresh staging setup, this empty value is deserialized verbatim by GetEstatesConfig and then assigned directly to CreateEstateUserRequest.Password; merchant passwords in the same file follow the same path. Unlike the identity-server secrets, no environment-variable substitution is applied, so setup either fails user creation or creates users that cannot authenticate. The development and production setup files contain the same regression.
Useful? React with 👍 / 👎.
| IdentityServerConfiguration identityServerConfiguration = StringSerialiser.Deserialise<IdentityServerConfiguration>(identityServerJsonData); | ||
| ApplyIdentityServerSecrets(identityServerConfiguration); |
There was a problem hiding this comment.
With the currently selected Mode.EstateSetup, Main still calls this method before the mode switch, and this new call requires secrets for every API resource and client. Consequently, an estate-only setup now aborts unless all identity-server environment variables are present, even though that mode only needs the service-client secret. Load and populate this configuration only for Mode.SecuritySetup, or avoid requiring unrelated secrets in other modes.
Useful? React with 👍 / 👎.
| [string]$BaseUrl = "http://localhost:2113", | ||
| [string]$Username, | ||
| [string]$Password | ||
| [System.Management.Automation.PSCredential]$Credential |
There was a problem hiding this comment.
The pull-request workflow runs ScheduledTasks/Tests, but Scavenge.Tests.ps1 still invokes this script with -Username and -Password and expects a Basic authorization header. Those arguments no longer populate anything in the script, so $Credential remains null and the authorization assertion fails. Retain compatibility or update the checked-in callers and tests to construct a PSCredential in the same change.
Useful? React with 👍 / 👎.
| await RecordMerchantRunResultAsync( | ||
| runId, | ||
| merchant, | ||
| scheduledRunUtc, | ||
| MerchantRunStatuses.Failed, | ||
| string.Join(Environment.NewLine + Environment.NewLine, contractFailures.Select(ex => ex.ToString())), | ||
| cancellationToken); |
There was a problem hiding this comment.
When one or more contracts fail, this inserts a failed MerchantRunRecord and then throws an AggregateException; the outer catch at line 32 catches that exception and inserts another failed record for the same run. RecordMerchantRunResultAsync always adds a new row, so every contract-level failure now produces duplicate history/dashboard entries and can consume two different completion timestamps. Restore the previous recorded-result guard or let only one layer persist the failure.
Useful? React with 👍 / 👎.
| var pendingLookup = fileSendRows | ||
| .Where(record => (bool)record.ProcessingCompleted == false && !string.IsNullOrWhiteSpace((string)record.MerchantId)) | ||
| .GroupBy(record => (string)record.MerchantId) | ||
| .ToDictionary(group => group.Key, group => group.Count(), StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
For merchants with failed file-send records, this in-memory predicate counts those records as pending because failed records also retain ProcessingCompleted == false. The previous database query required Status == Succeeded, matching the actual status-polling predicate, so the operations dashboard now reports failed uploads as files awaiting completion checks. Add the succeeded-status condition back to this lookup.
Useful? React with 👍 / 👎.
Closes #637
Closes #635
Closes #634
Closes #631
Closes #627
Closes #624
Closes #621
Closes #620
Closes #617
Closes #610
Closes #606
Closes #602
Closes #598
Closes #590
Closes #508
Closes #507
Closes #506
Closes #505
Closes #502
Closes #494
Closes #486
Closes #485
Closes #483
Closes #478
Closes #477
Closes #466
Closes #464
Closes #448
Closes #447
Closes #444
Closes #440