openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.22k stars 918 forks source link

Add filtering functionality to user notes page #5255

Closed kcne closed 3 weeks ago

kcne commented 1 month ago

This PR addresses #832 by adding sorting and filtering functionality to user note pages. This PR is also an alternative, or better put expanding #5239 so that filtering and sorting functionality can be expanded further on if needed.

Key Changes:

Tests for added methods and scopes have been added to ensure that these features work as expected across different scenarios, including handling of date ranges and note types.

Screenshots:

Screenshot 2024-10-08 at 16 47 57 Screenshot 2024-10-08 at 16 48 22 Screenshot 2024-10-08 at 16 48 34
AntonKhorev commented 1 month ago

About the date pickers:

I wanted to add them to our standard pagination because we already have date indexes. Then we wouldn't need them here, specifically in this notes filer. But this plan haven't worked so far because:

kcne commented 1 month ago

Thank you for the detailed background and context, Anton. I agree that using date pickers makes sense if we’re filtering notes by dates, especially since the current setup isn’t relying on standard pagination. Having date pickers included in standard pagination for entries with created_at and updated_at attributes makes sense in a way but I'm not sure how long would it take for something like this to be implemented and merged.

As for the next steps, I’m not sure whether it’s better to get this PR merged as is and refactor later when the notes move to standard pagination, or if we should address the pagination issue on user note pages first and then refactor this PR accordingly.

On a side note, I’ve managed to implement sortable columns on a table UI end for created_at and updated_at locally, but I’ll hold off on submitting that until this PR is merged to keep things cleaner and easier to review.

AntonKhorev commented 1 month ago

As for the next steps, I’m not sure whether it’s better to get this PR merged as is and refactor later when the notes move to standard pagination, or if we should address the pagination issue on user note pages first and then refactor this PR accordingly.

You can make a pull request that just adds the status filter. That doesn't depend on pagination and shouldn't tank the performance because:

AntonKhorev commented 1 month ago

On a side note, I’ve managed to implement sortable columns on a table UI end for created_at and updated_at locally

I looked again at why similar things were rejected in the past like #1656 and there doesn't seem to be any clear reason.

AntonKhorev commented 1 month ago

Besides the double-join db query, there's couple more issues with the interaction type: "blue on blue" colors and whether the submitted/commented classification makes sense at all (what if I closed a note without a comment?). I wanted to try opened/closed/commented instead.

kcne commented 1 month ago

Thank you for review @AntonKhorev

Few updates since last commit:

  1. I've removed interaction type and refactored note type to have opened/closed/commented classification now.

  2. After digging a bit deeper I've managed to implement the query using NOT EXISTS command which should perform slightly better avoiding joins. Since note comments table already have index on author_id as first entry in composite index -> # index_note_comments_on_author_id_and_created_at (author_id,created_at) this query should be performing okay now.

  3. When you mentioned "blue on blue" colors I'm not sure if this is something I've introduced in this PR since when I toggle dark mode on my end everything is looking fine.

Screenshot 2024-10-24 at 14 53 14
kcne commented 1 month ago

As for splitting this into multiple smaller PRs, I would prefer we merge this as a complete feature/functionality. Adding filters one by one would significantly increase the time required and might leave the feature feeling incomplete. This PR covers filtering part for intended functionality cohesively, and it seems better to refine and adjust post-merge if needed.

AntonKhorev commented 1 month ago
  1. When you mentioned "blue on blue" colors I'm not sure if this is something I've introduced in this PR

Obviously you didn't introduce it. That comment was made in July.

since when I toggle dark mode on my end everything is looking fine.

Check it when the note is colored as "submitted".

AntonKhorev commented 1 month ago

Adding filters one by one would significantly increase the time required

Time for what? For PR review?

and might leave the feature feeling incomplete.

Are you sure? Is it complete with exactly the filters you added and not some other filters?

kcne commented 1 month ago

Time for what? For PR review?

Exactly, since each PR would need to be reviewed sequentially, it would prolong the overall process.

Are you sure? Is it complete with exactly the filters you added and not some other filters?

Yes, the goal was to implement the filters that were most commonly requested and improve the feature based on that feedback. However, if you have any suggestions for additional filters that could enhance the usability for mappers, I’m definitely open to discussing and considering those ideas too.

AntonKhorev commented 1 month ago

Yes, the goal was to implement the filters that were most commonly requested and improve the feature based on that feedback.

Where are these requests? For example #832 wants status filter but doesn't say anything about date ranges. Why would a status filter be incomplete without a date range filter?

Also https://github.com/openstreetmap/openstreetmap-website/issues/1534#issuecomment-535840056.

kcne commented 3 weeks ago

I opened new PR #5297 according to suggestion here. Closing this as not planed for now.

You can make a pull request that just adds the status filter. That doesn't depend on pagination and shouldn't tank the performance because:

- status is checked anyway to see if the note is not hidden

- we're doing a similar thing in the api