PER-10637: Show displayTime in file list#1067
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1067 +/- ##
==========================================
+ Coverage 51.64% 51.66% +0.02%
==========================================
Files 354 354
Lines 12055 12067 +12
Branches 2166 2172 +6
==========================================
+ Hits 6226 6235 +9
- Misses 5604 5605 +1
- Partials 225 227 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d81d1df to
5e5d8d8
Compare
aasandei-vsp
left a comment
There was a problem hiding this comment.
I cannot approve this PR, because I have found some issues. Could you please have a look and check if I am on the right track with these, please?
There is a bug on FE, which I fixed(see latest commit). If a folder is added, the displayTime will be an interval and the pipe does not know how to show it. What I did was to extract the start date from the interval and then feed it to the pipe, so it shows correctly. DisplayDT is also like the start date for folders, so this approach seems correct to me. See the bug in the app in the image bellow:
The second issue I found is related to sorting by date, which I cannot fix on FE, as we get the sorting from /navigateLean. The problem is that the sorting is still happening by displayDT and not by displayTime. This seems a difficult one to solve, as displayDT and displayTime do not seem to be in sync and we are also not sure which one we'll be showing in the list. See the image bellow:
|
Thanks @aasandei-vsp, looks like a great implementation for ranges! I have a back-end PR in progress for sorting: https://github.com/PermanentOrg/back-end/pull/892. |
There was a problem hiding this comment.
Pull request overview
Updates file-list date rendering to prefer the new backend displayTime field (including EDTF intervals) while improving prDate handling for date-only strings.
Changes:
- Add
EdtfService.getEdtfIntervalStartDate()and tests to extract an interval’s start for display. - Update
PrDatePipe(and tests) to treat date-only strings (YYYY-MM-DD) as having no time portion. - Update file-list item rendering to use
displayTime(or interval start) with fallback todisplayDT, and add component tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/shared/services/edtf-service/edtf.service.ts | Adds helper to extract interval start date from an EDTF string. |
| src/app/shared/services/edtf-service/edtf.service.spec.ts | Adds unit tests for the new EDTF interval-start helper. |
| src/app/shared/pipes/pr-date.pipe.ts | Adds special-casing for date-only strings (no time output). |
| src/app/shared/pipes/pr-date.pipe.spec.ts | Adds coverage for date-only inputs (with/without timezone, date vs time part). |
| src/app/file-browser/components/file-list-item/file-list-item.component.ts | Uses EDTF helper to compute a startDisplayTime fallback value. |
| src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts | Adds tests for displayTime precedence and interval-start behavior. |
| src/app/file-browser/components/file-list-item/file-list-item.component.html | Switches displayed date/time to use startDisplayTime instead of displayDT. |
Comments suppressed due to low confidence (1)
src/app/file-browser/components/file-list-item/file-list-item.component.ts:264
startDisplayTimeintroduces a new display source (displayTime), but thedatefield used by the public-archive header is still derived fromitem.displayDTinngOnInit(). This can make the public-archive date disagree with the date shown elsewhere for the same item, and date-only values (YYYY-MM-DD) can also be shifted by the viewer’s local timezone when passed throughDate/toLocaleString.
Consider deriving date from startDisplayTime, and formatting date-only strings in a timezone-stable way (UTC).
get startDisplayTime(): string {
return (
this.edtfService.getEdtfIntervalStartDate(this.item.displayTime) ||
this.item.displayDT
);
}
async ngOnInit() {
this.recordThumbnailUrl = GetThumbnail(this.item);
const date = new Date(this.item.displayDT);
this.date = getFormattedDate(date);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{ startDisplayTime | prDate: item.TimezoneVO : 'date' }} | ||
| @if (item.dataStatus > 0) { | ||
| <span @ngIfFadeInAnimation>{{ | ||
| item.displayDT | prDate: item.TimezoneVO : 'time' | ||
| startDisplayTime | prDate: item.TimezoneVO : 'time' | ||
| }}</span> |
| @if (item.dataStatus > 0) { | ||
| <div class="date-bottom" @ngIfFadeInAnimation> | ||
| {{ item.displayDT | prDate: item.TimezoneVO : 'time' }} | ||
| {{ startDisplayTime | prDate: item.TimezoneVO : 'time' }} | ||
| </div> |
slifty
left a comment
There was a problem hiding this comment.
This looks good -- copilot flagged an edge case that can result in an empty element that may create a visual effect; I defer to @cecilia-donnelly on whether we want to block on that, so I'm approving!
|
@slifty when doing this I didn't test all the possible EDTF types because I expect we'll do a second round of formatting before lifting the feature flag and letting people add uncertain dates. Currently no one can create a date without a time until we release the full feature. I don't think this behaves particularly well for e.g. year-only dates, either. I saw this PR more as a bugfix for the current state (where the date is mysteriously different from one place to the next) and didn't intend it to treat all the ways we'll need to format EDTF dates. I will add a ticket specifically about making sure we're doing formatting well here to the epic, though, so we don't forget! Does that make sense? |
|
The promised ticket: https://permanent.atlassian.net/browse/PER-10655 |
QA InstructionsQA Testing Instructions for PR: PER-10637SummaryThis PR modifies the Test Environment Setup
Test Scenarios
Regression Risks
Things to Watch For
These instructions should provide comprehensive coverage for testing this pull request. Generated by QA Instructions Action |
|
My take on asking Claude to write a nontechnical list. It doesn't know that there's no way to clear date or set a range yet, so I removed several of the suggested tests: Test SetupBefore running these tests, prepare the following items in your archive. You will need edit access to all of them. Files to prepare:
Folders to prepare:
TestsTest 1: File with no custom date shows the upload dateItem: File A (no custom date set) Steps:
Expected result: The date shown is the date and time when the file was uploaded to the archive. It should include a time of day. Test 3: File with a single date and time shows the date and timeItem: File C (custom date set with a specific time) Steps:
Expected result: The date and time shown match what you entered in the info panel. Test 7: Folder with no custom date shows the creation dateItem: Folder A (no custom date set) Steps:
Expected result: A date is shown for the folder (it should reflect when the folder was created or its default date). It should not be blank. Test 8: The file list updates immediately after editing a dateItem: Any file or folder you have edit access to. Steps:
Expected result: The date shown in the file list row updates to match the new date you just entered. Test 10: Date display is consistent in both list view and grid/thumbnail viewItem: File D (start date and end date both set) Steps:
Expected result: In both views, only the start date appears. It is the same date in both views. |
|
All of the above look correct to me and I also confirmed that the date is correct on first page load. |
Folder displayTime values are EDTF intervals (start/end), which the prDate pipe cannot parse and rendered as "Invalid date". Decompose the interval and display only the start date, falling back to displayDT when there is no resolvable start. Add EdtfService.getEdtfIntervalStartDate to extract the interval start (returning non-interval values unchanged and an empty string for open starts), and a startDisplayTime getter on the file list item that uses it with the displayDT fallback. Issue: PER-10637
e6c606e to
a3a5de2
Compare
|
@omnignorant verbally approved this. |
Removing this because @aasandei-vsp addressed some of her own feedback (thank you!) and the sorting was fixed in another PR (https://github.com/PermanentOrg/back-end/pull/892)


Change the date pipe to optionally take
displayTimeand fall back todisplayDT. This depends on https://github.com/PermanentOrg/back-end/pull/889.