feat: File versioning - WPB-21660 #3973
Conversation
… assets when modified
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 57c5809. ♻️ This comment has been updated with latest results. |
| ) | ||
| } | ||
|
|
||
| func `It returns a collection of WireCellsNodeVersion`() async throws { |
| #expect(result == WireCellsNodeVersion.mock) | ||
| } | ||
|
|
||
| func `It fails retrieving node versions`() async throws { |
There was a problem hiding this comment.
Praise: Using the new style tests.
| do { | ||
| try await localAssetRepository.downloadAsset(source: source) | ||
| cacheKey = try localAssetRepository.asset(nodeID: source.id)?.downloadState.cacheKey | ||
| } catch WireCellsLocalAssetRepositoryError.downloadAlreadyInProgress { |
There was a problem hiding this comment.
issue: This branch is not necessary as downloadAlreadyInProgress can no longer be thrown. I thought I had already deleted it, but perhaps not.
There was a problem hiding this comment.
yes, no longer here in latest commits, it's up to date.
| private func awaitDownload(id: UUID) async throws { | ||
| for await item in localAssetRepository.observeAsset(nodeID: id).values { | ||
| try Task.checkCancellation() | ||
|
|
||
| switch item?.downloadState { | ||
| case .downloaded: | ||
| return | ||
| case let .failed(error): | ||
| throw error | ||
| default: | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: This code can be deleted.
There was a problem hiding this comment.
removed, updated with latest commits
.../Sources/WireMessagingDomain/WireCells/Protocols/WireCellsLocalAssetRepositoryProtocol.swift
Outdated
Show resolved
Hide resolved
jullianm
requested review from
a team,
David-Henner and
netbe
and removed request for
a team and
WilhelmOks
| WireLogger.wireCells.error("Unable to restore node version: \(error)") | ||
| throw Failure.unableToRestoreNodeVersion |
There was a problem hiding this comment.
question: Do we have to throw and log here? What type of error is catched, consider if this can be logged publicly
samwyndham
left a comment
samwyndham
left a comment
There was a problem hiding this comment.
Nice work. A few comments to address but pre-approving as I will soon be on vacation
| state = .restoringVersion | ||
|
|
||
| // Keep the view visible for a couple of seconds to avoid a quick glitch. | ||
| try? await Task.sleep(nanoseconds: 2_000_000_000) |
There was a problem hiding this comment.
suggestion: Use try? await Task.sleep(for: .seconds(2))
question: Do you have any idea what is the cause of this glitch? 2 seconds is a long time. What does the user see during this period?
|
|
||
| private func makeVersionItem(version: WireCellsNodeVersion) -> FileVersionItem { | ||
| let title = version.modified.map(formattedItemDate) ?? "" | ||
| let subtitle = (version.ownerName ?? "") + " · " + (version.size.map(formattedFileSize) ?? "") |
There was a problem hiding this comment.
issue: The subtitle will be funny here if one of the components is missing. How about something like...
// rough code
[version.ownerName, version.size.map(formattedFileSize)].compactMap {. $0 }.joined(" · ")
| ) -> [Date: [WireCellsNodeVersion]] { | ||
| Dictionary(grouping: versions) { version in | ||
| let date = version.modified ?? Date.distantPast | ||
| let components = Calendar.current.dateComponents([.year, .month, .day], from: date) |
There was a problem hiding this comment.
question: can you use Date.startOfDay(for:) here?
| } | ||
| } | ||
|
|
||
| if canOpenVersionHistory { |
There was a problem hiding this comment.
issue: I think you might also need to check !viewModel.isInRecycleBin so that it doesnt show when in the recycle bin
|
issue: Could you also put this feature behind the |
Issue
This PR introduces File versioning, specifically the ability for a user to :
note: video taken before merging latest changes from develop also opening and viewing a version has been recently removed from the implementation
https://github.com/user-attachments/assets/6c2defce-cb7b-414b-94b8-00c069cd2d1a
Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: