ShopFloor - Refactoring

Refactor Findings

Running catalog of code smells, anti-patterns, fragility, and open questions discovered during the breadth-first documentation pass. This is the source list for picking refactor targets — not yet a refactor plan.

Items are grouped by area and tagged with severity:

• 🔴 Critical — actively risky to security, data integrity, or production stability. Fix before any next prod cycle.
• 🟠 High — material design problem that complicates every refactor. Worth its own dedicated effort.
• 🟡 Medium — meaningful improvement but not blocking. Bundle with related work.
• 🟢 Low — quality-of-life improvements and naming nits.

Each item links back to the doc where it was first surfaced.

---

1. Security & secrets

🔴 Secrets committed in appsettings.json

Source: 10-architecture.md §10

appsettings.json is tracked in git and contains live-looking values for DB passwords, Fishbowl/Mercado Libre/SendGrid/Seq API keys, Azure credentials, and the JWT signing secret (AppSettings:Token). Until rotated and moved to a secrets manager (User Secrets in dev, KeyVault/AWS Secrets Manager in prod), the entire commit history is a credential exposure.

Recommended fix:

1. Rotate every secret immediately.
2. Move to ASP.NET Secret Manager (dotnet user-secrets) for dev, KeyVault for prod.
3. Add appsettings.json to a “scrub-on-commit” pre-commit hook or git-secrets check.
4. Audit history with BFG / git-filter-repo to remove old values (note: rotation is still required even after history rewrite).

🟡 Loose Identity password policy

Source: 10-architecture.md §7

ASP.NET Identity is configured with no uppercase / digit / special-char requirements. Combined with the secrets exposure above, this widens the blast radius of any credential leak.

🟡 ResourceAccessHandler fragility

Source: 10-architecture.md §7

The handler reaches deep into context.Resource → RouteEndpoint without exception handling. A surprise route configuration could throw NullReferenceException and 500 the request.

---

2. Inventory integrity (high priority)

🟠 Part-consumption write-down appears unwired

Source: 02-unit-flow.md §6.6

When a part is used in a repair, RCReplacementParts records the association and ConsumptionRequest may be created — but no code path visibly decrements InventoryTracker.AvailableQuantity or sets Part.ConsumedByWorkTrackingID. GetAvailableKeypart() filters on ConsumedByWorkTrackingID == null, implying the field should be set, but no service method writing it was found.

Either:

• (A) the logic exists in a code path not traced (Hangfire job? trigger?) — find it.
• (B) it’s a real bug that’s been masked by manual inventory adjustments — fix it.

Recommended fix once confirmed:

• Centralize part consumption in a PartConsumptionService.Consume(partId, workTrackingId, reason) that atomically writes Part.ConsumedByWorkTrackingID, creates an InventoryTransaction with TransactionType="OUT", decrements InventoryTracker.AvailableQuantity, and emits an audit row.
• Add a daily reconciliation report comparing InventoryTracker.AvailableQuantity to COUNT(Part WHERE ConsumedByWorkTrackingID IS NULL). Any drift = a bug.

🟠 RecoveryEvent vs. HarvestingEvent distinction unclear

Source: 02-unit-flow.md §6.5

RecoveryEvent mirrors HarvestingEvent but joins to WarehouseID instead of a unit. Three hypotheses (legacy / manual stock-in / planned feature) — confirm which before any refactor touches either entity.

🟡 RCReplacementParts.HarvastingSN (sic)

Source: 02-unit-flow.md §6.6

A string column (“Harvasting” misspelled) intended to identify the donor unit. Should be a foreign key to either WorkTracking.WorkTrackingID (donor unit) or HarvestingEvent.HarvestingEventID.

---

3. Workflow engine

🟠 Branch routing has no unified mechanism

Source: 13-workflow-engine.md §6, 02-unit-flow.md §6.2

Routed transitions go through the dispatcher; branch transitions (to Repair Center, BER, Quarantine, etc.) bypass it and set WorkTracking.StationID directly inside various services. There is no service that captures why a unit jumped, who triggered it, or guarantees a WorkTrackingOperations audit row.

Recommended fix:

• Introduce StationTransitionService.Transition(workTracking, targetStation, reason, user) that wraps both routed and branched movements.
• Always writes a WorkTrackingOperations audit row.
• Centralizes the post-repair re-entry logic (see next item).

🟠 Post-repair re-entry is undocumented

Source: 02-unit-flow.md §6.2

When Repair Center marks a unit Repaired=true, no observed code path explicitly returns it to a specific Route step. Three hypotheses — operator re-scan, manual override, or some service I haven’t traced. Confirm with operators before refactoring.

🟡 FindIndex == -1 edge case in dispatcher

Source: 13-workflow-engine.md §11

If a unit’s current StationType isn’t in its Route’s RouteFlow, FindIndex returns -1. The dispatcher’s check candidateIdx == currentIdx + 1 becomes 0 == -1 + 1 → true, allowing a scan at the very first station from any unknown state. Worth a regression test.

🟡 Same StationType appearing twice in a Route

Source: 13-workflow-engine.md §11

FindIndex returns the first match, so the second occurrence is unreachable. Not believed to happen in practice — confirm.

🟡 No concurrency token on WorkTracking

Source: 13-workflow-engine.md §11

Two operators scanning the same unit simultaneously at different stations would last-write-wins. EF Core concurrency tokens are not configured.

🟡 Route-compliance check duplicated across station services

Source: 13-workflow-engine.md §5, 02-unit-flow.md §2

The same IsStationChangeNotRestrictionCompliant invocation pattern appears in 6+ station services. Should be enforced solely by the dispatcher.

🟡 WorkTrackingOperations write path unverified

Source: 13-workflow-engine.md §7

Worth a grep to confirm where (if anywhere) movement audit rows are written. The entity exists but the write path isn’t obvious from the services I traced.

---

4. Triage

🟠 Hardcoded tenant branch (“Dallas SL” vs. else)

Source: 02-unit-flow.md §4

TriageEvaluationService branches on tenantName == "Dallas SL" (line 294) with the author’s own comment //<-- HATE THIS HARDCODED THING. Replace with a tenant-configurable strategy table or per-project routing rules.

🟡 Hard-indexed evaluation order

Source: 02-unit-flow.md §4 smells

TriageEvaluationService.cs lines 401–404 sort evaluations alphabetically and rely on “Cosmetic” coming before “Functional”. Adding a third evaluation type breaks silently. Use explicit evaluation-type IDs.

🟡 Magic score thresholds (46 / 45 / 9)

The Mexicali-vs-Dallas branch logic and the Grade A pass-through hardcode 46, 45, and 9. Move to configuration or a per-project lookup table.

🟡 Three split entry points in TriageEvaluationService

CanBeProccesed, PromoteBasedOnGrade, and ProcessItemToStation form an unconsolidated routing tier. Callers must know which to call for which tenant. Unify into one orchestrator that selects strategy by tenant config.

🟡 Triage decision logic deserializes JSON in-memory

TriageEvaluationSubmission.SerializedSelections stores the full questionnaire response as JSON. Routing logic deserializes this blob in-memory for every decision. Normalize into a queryable table.

🟡 Inconsistent auto-fail logic

Functional eval triggers auto-fail on substring "Not Functional" or the autoFail flag. Cosmetic eval only checks the flag. Standardize.

🟢 Discrepancy entity exists but isn’t used in Triage

Discrepancy.cs has the shape of a Triage output (WorkOrderID, BOL, SKU, SerialNumber) but isn’t referenced by the modern Triage flow. Likely legacy.

---

5. RMA

🟠 No FK between RmaHeader and RepairCenter

Source: 02-unit-flow.md §5

Linkage is by string match on WorkTracking.SerialNumber == RepairCenter.SerialNumber. Recommend adding RepairCenter.RmaItemsRelationID (or routing through WorkTrackingID) for referential integrity.

🟡 No RmaOutcome field on WorkTracking

Source: 02-unit-flow.md §5 smells

You can’t tell whether an RMA was successfully re-processed or returned a second time. Add a status.

🟡 Reset logic duplicated

RMAControllerService.CreateNewRma, RMAControllerService.AddItemToExistingRMA, and RmaForOutOfSystemUnitsControllerService.GenerateRMA (old-path) all clear the same set of WorkTracking fields. Extract a shared WorkTracking.ResetForRMA() method.

🟡 Magic station IDs

item.StationID.Equals(21) and similar magic numbers should use StationTypeEnum constants.

🟡 RmaNotInsystemPreAlert.IsValid ambiguity

Can be true while ErrorDescription != "OK". Define clear status semantics.

🟢 Nothing prevents one unit being attached to multiple RmaHeaders

Add a uniqueness constraint or explicit business-rule check.

---

6. Repair Center

🟠 Dual error tracking — RepairCenter + RepairCenterError

Source: 02-unit-flow.md §6

Both tables track errors for the same unit but via different keys (WorkTrackingID vs. SerialNumber). Queries use different filters in different places. Risk of one being updated without the other.

🟡 Magic-string status values

RepairCenterError.Status uses literal strings like "PENDING" and "Issue Fixed". Convert to an enum.

🟡 Duplicate queries in availability lists

consuWithoutRepair and hasError (around RepairCenterStationControllerService lines 1903–1904) both scan the entire repair-center repository for a serial number; could be combined.

🟡 PendingPartsCatalog misnamed

Source: 02-unit-flow.md §6.3

It’s not a “parts to procure” list — it’s a BOM-linked reference catalog. Rename to RequiredPartsCatalog or BomKeypartsCatalog.

---

7. Imaging / spec capture

🟡 PromoteImageDownloadUnit vs. MoveFromWindowsTestSystem

Source: 02-unit-flow.md §3, 12-station-catalog.md §3

Two stations with overlapping purposes. Naming is misleading. ⚠️ Confirm whether they have meaningfully different uses (e.g., one for re-imaging) before consolidating.

🟡 SystemInformation* vs. Engineering* overlap

Source: 11-data-model.md §5

Two parallel spec hierarchies. TranslateEngineeringDataEntriesJob is disabled — the migration from Engineering to SystemInformation is incomplete. Either complete the migration or accept both and document why.

🟡 Legacy questionnaire entities (QuestionsTests, AnswersTest, AnswersHeaderTest, TestByModel)

Source: 11-data-model.md §6

Two parallel questionnaire systems exist. Confirm the legacy one is dead and remove.

---

8. Data model

🟠 String columns that should be foreign keys

Source: 11-data-model.md §17

Entity.Column	Should reference
Discrepancy.SerialNumber	WorkTracking.SerialNumber
Discrepancy.BOLFileIdentifier	WorkOrders.BOLFileIdentifier
Discrepancy.InboundPalletIdentifier	InboundPallets.InboundPalletIdentifier
WorkTracking.VendorID (string)	Vendor
RepairCenter.SerialNumber	WorkTracking.SerialNumber
RCReplacementParts.HarvastingSN (sic)	WorkTracking.SerialNumber (donor)

🟡 Missing [ForeignKey] attributes

Entity.Column	Implied target
OutboundPallets.WareHouseLocationID	WareHouseLocations
OutboundPallets.WarehouseID	Warehouse
SalesOrders.WorkTrackingID	WorkTracking

🟡 Likely-missing indexes

WorkTracking.SerialNumber, WorkTracking.SKU / ArrivedSKU / LabelSKU, InboundPallets.InboundPalletIdentifier, OutboundPallets.OutboundPalletIdentifier, RepairCenter.SerialNumber. See 11-data-model.md §17.

🟡 Duplicate entities — Warehouse.cs vs. Warehouses.cs

Two entities with near-identical names. Confirm which is canonical.

🟡 Vendor has no ProjectID

Vendors are global; everything else is project-scoped. Either confirm intentional (vendors shared across projects) or add scoping.

🟢 Inconsistent ID initialization on Discrepancy

Discrepancy.DiscrepancyID lacks = Guid.NewGuid() while every other ID Guid in the codebase has it. Inconsistency.

🟢 PurchaseOrder.ProjectID is nullable

Most ProjectID FKs are Required. PurchaseOrder.ProjectID is optional. Confirm intentional.

---

9. Architecture / DI / services

🟠 Hardcoded tenant names in business logic

Source: 10-architecture.md §11, 14-jobs-and-integrations.md §3–4

• TriageEvaluationService branches on "Dallas SL".
• SyncSalesAndInventoryWithWorkTrackingJob has two hardcoded tenant GUIDs.
• GenerateAndNotifyOfHpCoaReportJob hardcodes tenant 36650844-da39-4b72-bf8f-ec989db49a27.
• SalesInventoryUpdateJob maps facility codes (MX, CA, SL) to tenants via hardcoded string compares.

Adopt per-tenant configuration tables.

🟡 ServiceManager with 100+ Lazy<> properties

Source: 10-architecture.md §3

Cognitively heavy. Consider domain grouping: ServiceManager.Repair, ServiceManager.Sales, etc.

🟡 RepositoryBase<T> duplication

Source: 10-architecture.md §2

One variant for AuditLogDbContext, another for the main RepositoryDbContext. Collapse where possible.

🟡 IParameterDataSource — confirm all 7 implementations are live

Source: 10-architecture.md §8

Seven implementations are registered and resolved via IEnumerable<IParameterDataSource>. Confirm none are dead.

🟢 Migration gating uses string compare

Equals("true") rather than typed boolean. Cosmetic but inconsistent.

🟢 SlowQueryInterceptor 5000 ms threshold instantiated inline

Threshold comes from config but interceptor is built inline. Consider making the interceptor injectable.

🟢 StationTypeEnum is incomplete

Source: 12-station-catalog.md §2

Triage, Kitting, Packing, BER, Harvesting, Recovery, Sorting, QA, etc. have controller-services but no enum entry. Either complete the enum or document why specific types are excluded.

🟢 Filename typo: GradingStationControllerservice.cs

Missing capital S on Service. Rename when convenient.

---

10. Background jobs

🟠 Bare catch {} in jobs

Source: 14-jobs-and-integrations.md §4

AssignSystemVarianceCodeToNewUnitsJob:36, TranslateSystemInformationEntriesJob:36, BaseJob.EmailDevelopersOnError():60. Silent failures across job boundaries are a top cause of “the data looks weird but everything ran.” Catch specific exception types and log.

🟠 Silent failures in SalesInventoryUpdateJob

UpdateLineItemDetailAndWorkTracking (line 210) returns when WorkTracking not found; nothing surfaced. There should be a SalesImportReconciliationFailures table or alert.

🟡 No retry / circuit breaker for external API calls

BitRaser, Fishbowl, Mercado Libre, HP PCB — all are called without Polly / retry / circuit-breaker. A single API blip kills the day’s job.

🟡 No idempotency gate on FishbowlSalesImportJob

Relies on external ref dedup. Concurrent runs could double-import.

🟡 Per-order swallow in FishbowlSalesImportJob (line 123)

Errors on individual orders are logged but not retried.

🟡 Stubs in production codebase

ConvertSalesImportJob, TranslateEngineeringDataEntriesJob — both incomplete. Either complete them or remove from the codebase.

🟢 PurgeAuditLogJob disabled in place

The job is commented out; comment says audit now writes directly to Mongo. Remove the file to reduce confusion.

---

11. Open questions to confirm with operators / domain experts

These aren’t refactor items per se — they’re things I literally couldn’t determine from code alone. Resolving them affects refactor priorities.

#	Question	Where surfaced
1	What is StationTypeID == Deboxing(10) actually the only valid predecessor to Triage? Where does Sorting fit?	02-unit-flow.md §4
2	How does a Repaired=true unit actually return to the happy path?	02-unit-flow.md §6.2
3	What operational role does RecoveryEvent play vs. HarvestingEvent?	02-unit-flow.md §6.5
4	Where is Part.ConsumedByWorkTrackingID actually written?	02-unit-flow.md §6.6
5	What do IMC and FLIP grades mean operationally?	03-glossary.md grading
6	Does an AS-IS unit skip Cleaning/Grading entirely, or modified versions?	02-unit-flow.md §8
7	Are PromoteImageDownloadUnit and MoveFromWindowsTestSystem genuinely two stations?	02-unit-flow.md §8
8	When is WorkOrders.DoesBolNeedDigitalKeyInjection set, by what?	02-unit-flow.md §8
9	Is Pallet Station always present, or only for batch-shipping projects?	02-unit-flow.md §8
10	At exactly which station does StationCategory flip from Pre-FGI (9) to FGI (10)?	02-unit-flow.md §8
11	Does RepositoryDbContext ever bypass the Finbuckle tenant filter via IgnoreQueryFilters()?	10-architecture.md §4
12	Does the SQL AuditLog table still receive writes anywhere?	10-architecture.md §9
13	Does the dispatcher check Station.ProductionFlow matches the unit’s production line?	13-workflow-engine.md §9
14	What are the unsourced Notifications enum values (EnrollmentTestResults, DpkInventoryReport, UnitsByStationType) used for?	14-jobs-and-integrations.md §6

---

12. Suggested first-round refactor targets

These are the items most likely to deliver value with bounded blast radius, ordered by impact-vs-effort estimate.

1. 🔴 Secrets remediation — non-negotiable security item. Rotate, move to KeyVault, scrub history.
2. 🟠 Confirm and (probably) fix the part-consumption write-down. Inventory accuracy is foundational to everything else.
3. 🟠 Confirm post-repair re-entry mechanics — interview operators. Once known, write a test. This unblocks the “branch routing” refactor.
4. 🟠 Centralize branch routing into StationTransitionService — encapsulates routed + branched movements, audits WorkTrackingOperations, removes duplicated logic.
5. 🟠 Replace hardcoded tenant strings with configuration — one table-driven mechanism covers Triage routing, sales import facility mapping, and HP COA eligibility.
6. 🟡 Resolve SystemInformation* vs. Engineering* — pick one canonical hierarchy and finish the migration job (or remove the stub).
7. 🟡 Add [ForeignKey] attributes and string→FK conversions in Discrepancy, RepairCenter, RCReplacementParts, OutboundPallets, SalesOrders. Each is a single-PR fix; bundle as one EF migration.
8. 🟡 Triage routing strategy pattern — collapse CanBeProccesed, PromoteBasedOnGrade, ProcessItemToStation into one orchestrator with per-tenant strategies.
9. 🟡 Bare-catch sweep across jobs — replace with typed catches + structured logging.
10. 🟡 ServiceManager domain grouping — improves navigability for everyone reading the codebase, including future Claude sessions.

---

13. Items NOT in scope for first refactor

These were observed but should wait — either they’re cosmetic, they’re dependent on resolving higher-priority items, or they need operator confirmation first.

• Legacy questionnaire entity cleanup (need confirmation it’s dead).
• Warehouse.cs vs. Warehouses.cs consolidation (need confirmation which is canonical).
• Filename typos (GradingStationControllerservice.cs).
• StationTypeEnum completion (low value relative to effort).
• RepositoryBase<T> deduplication (low value; both work).
• PendingPartsCatalog rename (cosmetic).
• ASP.NET Identity password policy hardening (worth doing eventually but blocked by the secrets rotation above).

---

14. Methodology note

This document was produced from a breadth-first code reading pass. Specific file:line citations live in the underlying docs that surfaced each finding:

• 02-unit-flow.md — Triage / RMA / Repair Center / parts
• 10-architecture.md — DI / auth / middleware
• 11-data-model.md — entity-level smells
• 12-station-catalog.md — station-related smells
• 13-workflow-engine.md — dispatcher edge cases
• 14-jobs-and-integrations.md — job + external-integration smells

Any item marked ⚠️ in those documents represents an assumption that needs confirmation before being treated as a refactor target.