fix: correct edited shared link expiration time#22466
fix: correct edited shared link expiration time#22466tskoyo wants to merge 4 commits intoimmich-app:mainfrom
Conversation
danieldietzler
left a comment
danieldietzler
left a comment
There was a problem hiding this comment.
Have you tested this? From only looking at the code I don't think this will work. Also, just in general this seems like the wrong approach to me. IMO the expected behavior of that dropdown should be to show the option originally selected when creating the shared link, not the closest option to whatever time is remaining
|
@danieldietzler , I tested it. When you create a new shared link, the expiration information is stored in the However, it’s not straightforward to show the originally selected expiration option, since it’s stored as a specific timestamp. For example, if you set the expiration to 6 hours and check back after 2 hours, the remaining time would be 4 hours. So, some calculation is needed to display the correct expiration time. |
|
Right. I don't understand how you can deduct the original expiration interval from that timestamp, unless you also look at when that shared link was created. You don't do that |
| const expiresAt = DateTime.fromISO(editingLink?.expiresAt); | ||
|
|
||
| const now = DateTime.now(); | ||
| const remainingMs = expiresAt.toMillis() - now.toMillis(); |
There was a problem hiding this comment.
Here instead of using now.toMillis() can we use createdAt column?
also expiresAt - createdAt is not exactly 7 days (it's 10-20 seconds less than 7 days). add 30s-1m to expiresAt and now your old logic works.
There was a problem hiding this comment.
Tbh I feel like any frontend change here is kind of not ideal. I am wondering if we should just store the interval in the database, instead of/in addition to expiresAt. I'll bring it up in the team
| albumId = editingLink.album?.id; | ||
| assetIds = editingLink.assets.map(({ id }) => id); | ||
|
|
||
| const expiresAt = editingLink.expiresAt ? DateTime.fromISO(editingLink.expiresAt) : DateTime.fromMillis(0); |
There was a problem hiding this comment.
I think it'd be nicer to encapsulate all this new logic in a function like initialExpirationOption to help organize the code
benmccann
changed the title
|
This PR looks to be targeting similar functionality as #22274 |
|
Closing per Ben's comment. If it does actually fix a different issue, let us know and we can re-open |
4 participants
Description
When user wants to create a shared link and sets the link to expire in 1 day (for example), it wouldn't work when user wants to visit the
/shared-linkspage.Fixes # (issue)
How Has This Been Tested?
expire aftersome time/shared-linksExpire innowScreenshots (if appropriate)
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
...