Skip to content

Store the state of the computed filters#4121

Merged
tdonohue merged 3 commits intoDSpace:dspace-7_xfrom
arvoConsultores:DS-4097
Apr 11, 2025
Merged

Store the state of the computed filters#4121
tdonohue merged 3 commits intoDSpace:dspace-7_xfrom
arvoConsultores:DS-4097

Conversation

@sergius02
Copy link
Contributor

@sergius02 sergius02 commented Mar 31, 2025

References

Description

Store the state of the computed filters in the array finalFiltersComputed.

Instructions for Reviewers

The counter filtersWithComputedVisibility doesn't store the count of filters computed when you change between options in the select box in /mydspace, causing the ngIf in the HTML is always false, and no filters options render on the web.

List of changes in this PR:

  • First, countFiltersWithComputedVisibility now store the count every time a new filter is "computed" till it reach the maximum filters configured.
  • Second, added some utility functions to find the current and final counter, and a function to update the count.
  • Third, update the ngIf, to use the new functions.
  • Fourth, removed the trackBy: trackUpdate in the ngFor because when you enter in /mydspace from the link in the dropdown it seems that it interferes.

How to test:

  1. Enter in /mydspace from URL or from the link in the dropdown when you are logged in.
  2. Change between options in the select box from the sidebar
  3. The filters should be loaded without problem

Checklist

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using npm run lint
  • My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@kshepherd
Copy link
Member

@alanorth does this address the problem you've seen in 7.6.3?

@alanorth
Copy link
Contributor

alanorth commented Apr 2, 2025

@kshepherd I will try to test it this week. I still haven't managed to reproduce the bug reliably, but I see it when browsing.

@alanorth
Copy link
Contributor

alanorth commented Apr 3, 2025

I found a way to reproduce a related issue in Discovery search (see comment in #4097) and this PR fixes it.

+1 from me on functionality.

@ovali
Copy link

ovali commented Apr 3, 2025

This PR fixes the bug with the filters on the mydspace page for my 7.6.3 based branch. Thumbs up and thank you for providing this.

@tdonohue tdonohue added bug port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Apr 3, 2025
@tdonohue tdonohue moved this to 👍 Reviewer Approved in DSpace 9.0 Release Apr 3, 2025
@tdonohue tdonohue added the port to main This PR needs to be ported to `main` branch for the next major release label Apr 3, 2025
@tdonohue tdonohue self-requested a review April 3, 2025 14:58
@ovali
Copy link

ovali commented Apr 8, 2025

Anyone willing to review and approve this ?

@tdonohue
Copy link
Member

tdonohue commented Apr 8, 2025

@sergius02 : Could we get a version of this PR created for the main branch? That'd help me in my testing.

It appears (from discussion in this PR and on the ticket) that this bug is found to impact 7.x, 8.x and main. It sounds like a few people have found this to work for 7.x. Before we merge it though, I'd like to see a separate PR created for main, as that'd allow me to more easily verify the behavior there (I cannot get this PR to work with main as it has major merge conflicts there).

In the meantime, anyone else is welcome to provide your reviews/approval on this PR. Reminder that we accept reviews from anyone, and community reviews can help us get PRs merged more rapidly.

@alanorth
Copy link
Contributor

alanorth commented Apr 8, 2025

For what it's worth, I'm running this in production on DSpace 7.6.3. I didn't notice the bug on the MyDSpace page, but this fixes a bug that I showed with filters on the normal Discovery search page. Surprised nobody noticed that one yet.

@sergius02
Copy link
Contributor Author

@sergius02 : Could we get a version of this PR created for the main branch? That'd help me in my testing.

Ok, I'll try to do a port for the main branch

@sergius02
Copy link
Contributor Author

I've made a PR for the main branch here #4160

Copy link
Contributor

@FrancescoMolinaro FrancescoMolinaro left a comment

Choose a reason for hiding this comment

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

Hi @sergius02 , thanks for these changes, the PR looks good to me, I have just a small change request, if you could please have a look that would be great.

Thanks!

@github-project-automation github-project-automation bot moved this from 👍 Reviewer Approved to 👀 Under Review in DSpace 9.0 Release Apr 10, 2025
@saschaszott
Copy link
Contributor

@sergius02 , thanks for your contribution. This PR fixes the UI freeze, but it does not solve the following problem: after switching the Discovery Configuration in the dropdown, the frontend still requesting filters (facets) from the backend that are no longer valid for the newly selected Discovery Configuration? This is ultimately the reason for the 400 Bad Request errors returned by the backend (can be found in the JS console or in the DSpace backend log file dspace.log). Do you have any idea how to solve this problem?

Copy link
Contributor

@FrancescoMolinaro FrancescoMolinaro left a comment

Choose a reason for hiding this comment

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

Hi @sergius02 , thanks again for your changes, I gave a second look to the PR and found a small issue on the component, is not related to your code changes but I think is better if we address it in this PR, could you please have a look?

Thanks in advance!

Copy link
Contributor

@FrancescoMolinaro FrancescoMolinaro left a comment

Choose a reason for hiding this comment

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

Hi @sergius02 , thanks again for the prompt response.
I think this looks great now and to me is ready to be merged.

@tdonohue tdonohue removed the port to main This PR needs to be ported to `main` branch for the next major release label Apr 11, 2025
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @sergius02 ! I tested this via the main port in #4160. It fixes the main bug in the UI, so it looks good to me. I'm merging this with +2 votes (and some other testers -- thanks all).

I've also flagged this as port to dspace-8_x in order to attempt an automatic port to dspace-8_x. If that automatic port fails, then we may need a manual port.

@github-project-automation github-project-automation bot moved this from 👀 Under Review to 👍 Reviewer Approved in DSpace 9.0 Release Apr 11, 2025
@tdonohue tdonohue moved this to 👍 Reviewer Approved in DSpace Maintenance (9.x, 8.x, 7.6.x) Apr 11, 2025
@tdonohue tdonohue added this to the 7.6.4 milestone Apr 11, 2025
@tdonohue tdonohue merged commit f5df726 into DSpace:dspace-7_x Apr 11, 2025
19 of 21 checks passed
@github-project-automation github-project-automation bot moved this from 👍 Reviewer Approved to ✅ Done in DSpace Maintenance (9.x, 8.x, 7.6.x) Apr 11, 2025
@dspace-bot
Copy link
Contributor

Backport failed for dspace-8_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-8_x
git worktree add -d .worktree/backport-4121-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-4121-to-dspace-8_x
git switch --create backport-4121-to-dspace-8_x
git cherry-pick -x 7dd6ab79ffcb26a15b825b5eda46ce1aebc93370 4f42c1e95f9859f8997e2a290263822237b41f8a dc3e84274759bb29b5165dbad4eea5828d65705e

@tdonohue
Copy link
Member

@sergius02 : This was unable to be automatically ported to 8.x (see dspace-bot message above). Could you please create a PR against dspace-8_x in order to apply these changes to that release?

@sergius02
Copy link
Contributor Author

@tdonohue I've created a new PR with the port at #4179

@tdonohue tdonohue removed the port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release label Apr 11, 2025
@sergius02 sergius02 deleted the DS-4097 branch May 26, 2025 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

7.6.3 / 8 - Search facet filters disappear on mydspace page under circumstances

8 participants