Skip to content

Filter does not automatically apply after searching #2418

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

Open
Incogdino opened this issue Apr 14, 2025 · 4 comments
Open

Filter does not automatically apply after searching #2418

Incogdino opened this issue Apr 14, 2025 · 4 comments

Comments

@Incogdino
Copy link

Tell us about your environment

  • RepoSense Version: v3
  • OS and Version: Windows 11 Pro
  • Web Browser and Version (if applicable):
    Chrome

Please include the steps to reproduce the bug.

20250414-1353-11.1705418.mp4
  1. Search for a seperate team with the 'merge all groups' checkbox checked

What was expected to happen?

Result should show with the filter applied

What actually happened? Please include a screenshot of the output.

If possible, include the URL to your RepoSense report or log files (if any).

@CYX22222003
Copy link
Contributor

CYX22222003 commented Apr 14, 2025

Thanks for pointing out this potential feature flaw!

The repos to condense is determine by the URL field mergeGroup. The param hash will associate a list of repo urls which should be included in the dashboard display. And after entering the search keyword, the param hash should remain constant other than the search field.

Based on my investigation if the user filter then merge the group, only the filtered repos will be condensed (in this case only repos with "W13-3" is actually merged). When user continue to filter another repo, the hash mergeGroup will not be updated. Hence, the new filtered file with keyword "W13-4" will not be condensed.

Here is the URL after searching for "W13-3"

https://nus-cs2103-ay2425s2.github.io/tp-dashboard/?search=W13-3&sort=groupTitle%20dsc&sortWithin=title&since=2025-02-21&timeframe=commit&mergegroup=AY2425S2-CS2103T-W13-3%2Ftp%5Bmaster%5D&groupSelect=groupByRepos&breakdown=false

Here is the URL after searching for "W13-4"

https://nus-cs2103-ay2425s2.github.io/tp-dashboard/?search=W13-4&sort=groupTitle%20dsc&sortWithin=title&since=2025-02-21&timeframe=commit&mergegroup=AY2425S2-CS2103T-W13-3%2Ftp%5Bmaster%5D&groupSelect=groupByRepos&breakdown=false

May be we can further discussion on how to improve the behavior of merging and filtering repos?

@Incogdino
Copy link
Author

@CYX22222003 Ahh okay thanks for explaining how it works.

Should the url change according to the filter settings be more intuitive in this case? Are there any upsides to keeping the param hash of the previous search result as compared to just applying it to every new search result? Since as a user, I would expect the selected settings to still be applied to my search results.

@CYX22222003
Copy link
Contributor

@CYX22222003 Ahh okay thanks for explaining how it works.

Should the url change according to the filter settings be more intuitive in this case? Are there any upsides to keeping the param hash of the previous search result as compared to just applying it to every new search result? Since as a user, I would expect the selected settings to still be applied to my search results.

To clarify the previous search result is not kept, only the mergeGroup result is kept. And this field actually will be persistent even if you clear the search field. It seems that the current behavior of merging all groups just make an attempt to update the current filtered group and update the hash.

I think the suggestion by @Incogdino indeed has a good point. A possible way is to keep the setting for merged setting in a Boolean variable and pass it around using URL hash. So every time can update the merged groups in c-summary.vue based on its existing values and the flag.

However, I am not sure whether this implementation will conflict with any current implementation or violate any assumptions and principles.

@sopa301 @jonasongg @joeng03 @Airiinnn Any idea about this issue?

@sopa301
Copy link
Contributor

sopa301 commented Apr 15, 2025

From the way I see it, the state of the report and the configurations should match at all times. We should change the behaviour in one of two ways:

  1. (Recommended) Ensure the groups are merged after a change in the search bar. I don't recall any principles being enforced for the current implementation, so maybe someone else can chime in. Otherwise, it's up to the implementer to decide the implementation.
  2. (Easier, but introduces feature flaw) Uncheck the merge all groups checkbox after each search is applied.

For classification purposes, I'd agree that this is a bug, since the groups aren't merged as it claimed to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants