[RyoKusnadi]

Summary

Scheduled report warning/failure lines showed an empty attribute name, a value of 0 and no threshold, e.g. Attribute 3 () = 0. populateAlerts never set AttributeName or Threshold, and used GetTransformedValue(), which is 0 for most attributes.

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Related Issues

Fixes #1079

Changes Made

• Add alertAttributeDetails in reports/generator.go, which follows the detail page (detail.component.ts):
  • ATA: name from the metadata display name (smartctl Name only if there is no metadata, then the attribute ID); value from ThresholdValue (normalized, transformed or raw, so temperature shows 40 instead of the packed raw bytes); the drive threshold is shown only for normalized attributes or attributes without metadata.
  • Device statistics and FARM: metadata threshold first, drive-reported threshold as fallback (same as their status checks).
  • NVMe and SCSI: Value, drive threshold, metadata display name. Their "no threshold" marker -1 is reported as 0, so the text, HTML and PDF reports all show no threshold instead of -1.
• Add an exported SmartAtaAttribute.ThresholdValue that calls the existing thresholdValue, so the report reuses it instead of a third copy of the switch. thresholdValue stays, since fix(notify): compare evaluated value for repeat-only-on-change notifications #1086 uses it.
• Add tests: the report-level test, plus a table test for transformed ATA, raw ATA with a threshold, ATA without metadata or name, devstat and FARM metadata thresholds, the -1 marker, and SCSI.

Testing

• I have tested this locally
• I have added/updated tests that prove my fix/feature works
• All existing tests pass

go test ./webapp/backend/pkg/reports/... ./webapp/backend/pkg/models/measurements/... passes. Before the change the new cases fail (for example attribute 194 printed 65576 instead of 40, attribute 10 printed a threshold of 97 next to a raw value of 2). Not run: the full suite or a live scheduled report.

Checklist

• My code follows the project's code style
• I have performed a self-review of my code
• I have commented my code where necessary (particularly complex areas)
• I have updated the documentation if needed
• My changes generate no new warnings
• No console.log, debug statements, or commented-out code

[Starosdev]

Thanks for picking this one up, and for the test that pins the original bug.

Reviewed at head 560071e. Core fix is right — the fields existed (reports/models.go:104) and both formatters already render them, so this was purely a generator gap. Source-traced only; no build or test run. Five issues, all in alertAttributeDetails.

1. transformed ATA attrs report packed raw bytes. Only Normalized is special-cased, so attributes 9, 188 and 194 fall through to RawValue. Attribute 194's transform is rawValue & 0b11111111 (thresholds/ata_attribute_metadata.go:1033), so a 194 alert with raw 65576 prints 65576 where the UI shows 40C. Both existing consumers use TransformedValue here: measurements/smart_ata_attribute.go:229-238 and detail.component.ts:387-391. Prefer exporting that existing selector over a third copy of the switch.

2. ATA threshold printed against a raw-scale value. smartctl thresh applies to the normalized Value, but it is set for every ATA attribute and formatter_text.go:141 prints it whenever non-zero. Attribute 10 with raw 2, normalized 100, thresh 97 renders = 2 [threshold: 97]. The UI shows thresh only when display_type === 'normalized' (detail.component.ts:440-447); match that.

3. devstat and FARM lose their real threshold. Both prefer the metadata threshold and fall back to the drive value (smart_ata_devstat_attribute.go:101-104, smart_farm_attribute.go:91-94); the PR reads attr.Threshold only. devstat_7_8 fails against metadata threshold 100 while attr.Threshold is 0, so the threshold is dropped from the line. Mirror the fallback.

4. ATA name and value diverge from the UI (minor): the smartctl Name wins over metadata display_name, so labels read Spin_Up_Time against the UI's Spin Up Time (detail.component.ts:361-367); an attribute with neither a Name nor metadata still yields an empty name, since default: name = attrID only covers an unknown Go type; and for an attribute missing from AtaMetadata the value stays RawValue where the UI returns normalized value (detail.component.ts:393-406).

5. Test leaves the risky paths uncovered (minor): it does bind the original bug and is not tautological, but statuses are hand-set rather than from PopulateAttributeStatus, and transformed ATA, raw ATA with a threshold, devstat/FARM thresholds, SCSI and missing metadata are all untested. A transformed case plus a no-threshold assertion on a raw attribute would have caught 1 and 2.

[RyoKusnadi]

Thanks for the careful review! Pushed 89faa38 for all five points. I also updated the PR description.

1. Transformed ATA attributes: I exported the existing ThresholdValue selector and use it, so attribute 194 now shows 40 instead of 65576.
2. ATA threshold: it is now shown only for normalized attributes (or attributes with no metadata), like the detail page. Attribute 10 no longer prints [threshold: 97] next to a raw value.
3. devstat and FARM: they now use the metadata threshold first and the drive threshold as fallback, so devstat_7_8 keeps its 100.
4. Name and value: ATA uses the metadata display name (Spin-Up Time), then the smartctl name, then the attribute ID, so the name is never empty. An attribute with no metadata reports the normalized value.
5. Tests: added a table test for transformed ATA, raw ATA with a threshold, no metadata, no metadata and no name, devstat threshold and SCSI. These fail before the change and pass now. Statuses are still set by hand in the tests; I did not wire in PopulateAttributeStatus.

@Starosdev I've pushed the changes, could you take another look when you have time? Thanks!

[RyoKusnadi]

One small follow-up (019c143): instead of renaming thresholdValue to ThresholdValue, I kept the unexported method and added an exported ThresholdValue that calls it. #1086 uses thresholdValue, so the rename would have broken the build once both PRs are merged. I checked that the two branches build and pass tests together.

[Starosdev]

Thanks for the quick turnaround, and for catching that the rename would have broken #1086; that was the right call.

Reviewed at head 019c143b. Source-traced only; no build or test run. All five points from the previous round land:

1. Transformed ATA now reads attr.ThresholdValue(metadata.DisplayType), so 194 reports 40 instead of 65576, and the table case pins it.
2. The ATA threshold is cleared for non-normalized display types, matching detail.component.ts:443-447. Attribute 10 no longer prints [threshold: 97] against raw 2.
3. devstat and FARM prefer the metadata threshold. Your if metadata.Threshold != 0 form is equivalent to smart_ata_devstat_attribute.go:103-106 for every positive value, so devstat_7_8 keeps its 100.
4. The name is never empty. Metadata DisplayName wins, and all 87 AtaMetadata entries have one, so the smartctl Name fallback only fires for an attribute with no metadata. That is better than the detail page, which returns a literal Unknown Attribute Name in that case (detail.component.ts:361-367).
5. The table test covers transformed ATA, raw ATA with a threshold, missing metadata with and without a name, the devstat threshold and SCSI. FARM's metadata threshold is the one new branch still untested; a single case would close it.

Two things left.

1. The NVMe and SCSI no-threshold sentinel reaches the PDF (blocking). scsiAttributeNoThreshold is -1 (measurements/smart.go:548) and most NVMe attributes are constructed with Threshold: -1 (measurements/smart.go:479-491). alertAttributeDetails passes that through unchanged. formatter_text.go:141 hides it behind its > 0 guard, but formatter_pdf.go:275 formats alert.Threshold unconditionally, so a power_on_hours or errors_corrected_by_eccfast alert renders -1 in the Threshold column. The UI drops it (detail.component.ts:449: thresh === -1 ? '' : thresh).

Both formatters currently use 0 to mean "no threshold", which collides with the real zero on ATA attribute 4 (THRESH 000) and on NVMe critical_warning. Making AlertEntry.Threshold a *int64 with omitempty would carry absence honestly, let formatter_pdf.go render nil as blank, and drop the > 0 guard in formatter_text.go. It also needs threshold marked nullable: true in docs/openapi.yaml:3712-3714. 2.0.0 is unreleased (#965), so this is the cheap moment for that shape change. If you would rather keep the change small, normalising -1 to 0 inside alertAttributeDetails fixes the PDF and we can take the nullable field separately. Say which you prefer.

2. The metadata-absent ATA value now disagrees with #1086. This PR returns the normalized Value for an ATA attribute with no AtaMetadata entry, which matches the detail page and which your 999 case pins. measurements.AttributeEvaluatedValue in #1086 passes thresholds.AtaMetadata[id].DisplayType unconditionally, so a missing id gives "" and thresholdValue falls through to RawValue. Two helpers, two answers for the same attribute.

That is defensible. #1086 wants the value the status was evaluated on, and this PR wants the value a reader sees. It should be deliberate rather than incidental, though. Since #1086 already exports a package-level helper, the cleanest shape is for this PR to call it inside the metadata-present branch and drop SmartAtaAttribute.ThresholdValue entirely, keeping its own rule only for the metadata-absent case with a comment saying why it differs. That depends on merge order, so I am not asking you to rebase onto an unmerged branch: tell me which helper you think should own the ATA rule and I will sequence the merges around it.

Minor, no change requested: attributes 174, 204, 227 and 228 have metadata with no DisplayType, so they take the default branch and report RawValue where the detail page shows the normalized value. All four are Critical: false with no ObservedThresholds, so Scrutiny never flags them itself. They can only alert through a manufacturer SMART failure or a user override, and an override pins AttributeThresholdValue, which is the raw value. The raw value is the one consistent with the status reason there, so this reads as correct. Worth a sentence in the doc comment so the next reader does not take it for an oversight.

[RyoKusnadi]

Thanks, pushed 43adc8a.

1. Sentinel in the PDF: I went with the smaller option. alertAttributeDetails now turns the NVMe/SCSI -1 into 0, so the text, HTML and PDF reports all see "no threshold" and the API shape stays the same. Tests cover NVMe and SCSI -1; both failed before the change. If you would rather have the nullable *int64 threshold (so a real 0, like ATA attribute 4, can be told apart), I am happy to do that as a follow-up PR.

2. Which helper owns the ATA rule: I think measurements.AttributeEvaluatedValue from fix(notify): compare evaluated value for repeat-only-on-change notifications #1086 should own the metadata-present case. This PR keeps its own rule only for an attribute with no metadata (it shows the normalized value, as the detail page does), and a comment on alertAttributeDetails now says so. I did not call the helper here since fix(notify): compare evaluated value for repeat-only-on-change notifications #1086 is not merged; once it is, I can switch to it and drop ThresholdValue in a small follow-up.

3. FARM test and 174/204/227/228: added a FARM metadata-threshold case (no built-in FARM entry has one, so the test adds a temporary entry), and the doc comment now explains that those four attributes report RawValue on purpose.

go test and go vet pass on reports and measurements. I also updated the PR description for the -1 change.

[Starosdev]

Approving at head 43adc8af. Both items are addressed: the negative sentinel is clamped after the switch so it covers every protocol, and the comment records that measurements.AttributeEvaluatedValue owns the metadata-present ATA rule once #1086 lands.

Merge order is #1086 first, then this as it stands. Two follow-ups are yours to open after that: the nullable *int64 threshold, so a real zero (ATA attribute 4 at THRESH 000, NVMe critical_warning) can be told from no threshold and the PDF can render blank instead of 0, and the switch to AttributeEvaluatedValue dropping ThresholdValue.

Source-traced; CI on this head has not reported yet.

[RyoKusnadi]

Thank you! Noted on the merge order. Once #1086 and this one are in, I'll open the two follow-ups: the nullable *int64 threshold (so a real zero like ATA attribute 4 at THRESH 000 or NVMe critical_warning can be told apart from no threshold, and the PDF shows blank instead of 0), and switching to AttributeEvaluatedValue so ThresholdValue can be dropped.