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

Add filtering functionality to user notes page #5255

Closed

Conversation

kcne
Copy link
Contributor

@kcne kcne commented Oct 9, 2024

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:

  • Filter by Status: Users can filter notes by their status (open, closed, or all).
  • Filter by Note Type: View notes the user submitted or commented on (including those with anonymous comments).
  • Date Range Filtering: Users can filter notes based on a specified from and to date range.
  • Sorting Options: Sort notes by created_at or updated_at in ascending or descending order.

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

@kcne kcne marked this pull request as ready for review October 9, 2024 17:54
@kcne kcne force-pushed the 832-sorting-and-filtering-my-notes branch 2 times, most recently from 2e18a7c to 63ca327 Compare October 9, 2024 18:13
@kcne kcne marked this pull request as draft October 9, 2024 18:13
@kcne kcne force-pushed the 832-sorting-and-filtering-my-notes branch 2 times, most recently from e382dfa to 721f8d5 Compare October 9, 2024 18:51
@kcne kcne marked this pull request as ready for review October 9, 2024 18:59
@kcne kcne changed the title Sorting and filtering notes on the profile[TEST RUN] Add sorting and filtering functionality to user notes page Oct 9, 2024
@AntonKhorev
Copy link
Collaborator

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 kcne force-pushed the 832-sorting-and-filtering-my-notes branch 3 times, most recently from d87609f to d88ec30 Compare October 16, 2024 15:03
@kcne
Copy link
Contributor Author

kcne commented Oct 16, 2024

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.

@kcne kcne force-pushed the 832-sorting-and-filtering-my-notes branch from 84a01a3 to 8725d95 Compare October 16, 2024 16:08
@AntonKhorev
Copy link
Collaborator

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:

  • status is checked anyway to see if the note is not hidden
  • we're doing a similar thing in the api

<%= select_tag :status,
options_for_select([[t(".all"), "all"], [t(".open"), "open"], [t(".closed"), "closed"]], params[:status] || "all"),
:class => "form-select",
:onchange => "this.form.requestSubmit()" %>
Copy link
Collaborator

@AntonKhorev AntonKhorev Oct 17, 2024

Choose a reason for hiding this comment

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

onchange is not going to work with the csp_enforce: true setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally, this works as expected for me. Could you provide more details on how to reproduce this issue locally, or is this something that only occurs on production servers? Thanks.

@AntonKhorev
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

kcne commented Oct 21, 2024

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 kcne changed the title Add sorting and filtering functionality to user notes page Add filtering functionality to user notes page Oct 22, 2024
@kcne kcne force-pushed the 832-sorting-and-filtering-my-notes branch from cc6c281 to 900b915 Compare October 24, 2024 12:30
@kcne kcne force-pushed the 832-sorting-and-filtering-my-notes branch from 900b915 to ec76688 Compare October 24, 2024 12:35
@kcne
Copy link
Contributor Author

kcne commented Oct 24, 2024

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
Copy link
Collaborator

  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
Copy link
Collaborator

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
Copy link
Contributor Author

kcne commented Oct 24, 2024

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
Copy link
Collaborator

AntonKhorev commented Oct 24, 2024

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 #1534 (comment).

@AntonKhorev AntonKhorev added the notes Related to the notes feature label Oct 25, 2024
@kcne
Copy link
Contributor Author

kcne commented Oct 31, 2024

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

@kcne kcne closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes Related to the notes feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants