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 recent photo comments #876

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from
Open

Conversation

wilco375
Copy link
Contributor

Summary

Fixes #874

Copy link
Contributor

@lodewiges lodewiges left a comment

Choose a reason for hiding this comment

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

when commenting twice on a photo it shows up twice in recent images. This is unwanted behavior.
image

image

@wilco375
Copy link
Contributor Author

when commenting twice on a photo it shows up twice in recent images. This is unwanted behavior. image

image

Ah that's a good point. This might require some API changes to properly fix then. I'll look into it later!

Copy link
Contributor

@DrumsnChocolate DrumsnChocolate left a comment

Choose a reason for hiding this comment

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

what an interesting implementation oversight, haha
I trust the resulting implementation after the fix

@DrumsnChocolate
Copy link
Contributor

@wilco375 I don't think you need api changes; is it not possible to compute a list of photos and then run an operation of uniqueness over it? like a set or something like that

@wilco375
Copy link
Contributor Author

wilco375 commented Oct 11, 2024

@wilco375 I don't think you need api changes; is it not possible to compute a list of photos and then run an operation of uniqueness over it? like a set or something like that

Not if we want to get a fixed number of photos. Or we'd have to recursively get more photos from the backend until we have enough non-duplicate photos. But that would be nicer and more efficient to solve in a database query.

Either we get the photos directly and sort on date of latest comment => needs API addition of sorting by latest comment
Or we get latest comments but max one comment per picture => needs API addition of custom scope to filter out duplicate images
Or we repeatedly pull images from the API and do the filtering client side as described above, which might be very inefficient, especially if one image has a lot of recent comments (something that might not actually occur in practice but still)

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Oct 11, 2024

Right. I suppose the sort: '-updated_at', is passed to the API somehow? If that's the case, I suppose we could do some sort: '-commented_at', if we can define some commented_at property in the backend. You're the expert on rails

@DrumsnChocolate
Copy link
Contributor

@wilco375 bump

@wilco375
Copy link
Contributor Author

@wilco375 bump

Will look at it later. Currently busy with moving to another apartment so don't have much free time

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.

photo tags interfere with most recent comments
3 participants