Skip to content
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: remove duplicateIds on unique assets #13752

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Pranay-Pandey
Copy link
Contributor

fixes: #11637

Issue Overview

A bug was identified in the review duplicates utility where, after manually deleting a duplicate image, the corresponding group was still displayed with a single leftover image.

Changes

We validate the duplicates in duplicates utility page by checking if each duplicate set has atleast 2 assets. If not we update the assets to remove duplicates ids from them and do not show them on the page.

Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some code style comments, but looks good overall! I was thinking of adding a nightly check for this, but lazily checking when you fetch them is smarter.

@bo0tzz
Copy link
Member

bo0tzz commented Oct 26, 2024

Would this be better to do on the server side? It'd save a bunch of round trips etc.

@mertalev
Copy link
Contributor

Yeah, that makes sense to me.

Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, much better, thanks!

@@ -15,8 +15,24 @@ import { usePagination } from 'src/utils/pagination';
export class DuplicateService extends BaseService {
async getDuplicates(auth: AuthDto): Promise<DuplicateResponseDto[]> {
const res = await this.assetRepository.getDuplicates({ userIds: [auth.user.id] });

return mapDuplicateResponse(res.map((a) => mapAsset(a, { auth, withStack: true })));
const uniqueAssetIds: string[] = [];
Copy link
Member

@bo0tzz bo0tzz Oct 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming nitpick: These aren't necessarily unique, right (more accurately, all assetIds are unique)? Not sure what a better name would be though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

singletonDuplicateIds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate group with just one image after manually deleting the other(s)
4 participants