-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(server): asset count on meta tag of shared links #9107
fix(server): asset count on meta tag of shared links #9107
Conversation
@@ -185,7 +185,7 @@ export class SharedLinkService { | |||
|
|||
const sharedLink = await this.findOrFail(auth.sharedLink.userId, auth.sharedLink.id); | |||
const assetId = sharedLink.album?.albumThumbnailAssetId || sharedLink.assets[0]?.id; | |||
const assetCount = sharedLink.assets.length ?? sharedLink.album?.assets.length ?? 0; | |||
const assetCount = sharedLink.assets.length || sharedLink.album?.assets.length || 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When share link is a whole album, sharedLink.assets.length
is 0 and sharedLink.album?.assets.length
is the real asset count. But 0 ?? 10 ?? 0 === 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danieldietzler see another reason why ||
is better than ??
. Just causes headaches most of the time imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha! I think there are valid scenarios in which you specifically don't want ||
but yeah, by default I see your point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. We should probably add an e2e test for this. Is that something you want to take a look at? Essentially, make an http request to /share/<key>
and make sure the resulting html returns the correct meta tags. We could add this in e2e/src/api/specs/shared-link.e2e-spec.ts
.
sure, I'll add one |
Fixed #8605