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

Allow to perform action inside search results dataTable #2631

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

madryk
Copy link
Collaborator

@madryk madryk commented Jan 22, 2025

The main reason for the changes are because we want to render some buttons that have some actions attached inside pluggable/search-include-footer-fragment.xhtml. The pluggable/search-include-footer-fragment.xhtml is rendered inside f:dataTable component.
Currently when we click on the button then nothing happens. Reason behind this is that SearchIncludeFragment (bean connected to the f:dataTable) is request scoped. When we click the button SearchIncludeFragment was already removed from jsf context. But to perform the action jsf need for this bean to be present.

As a solution I've changed the bean that is connected to the f:dataTable to lastSearchValue which has broader scope.

Thread where the solution to the problem was found: https://stackoverflow.com/a/4128723

@madryk madryk requested a review from rscipien January 24, 2025 10:15
@madryk madryk requested a review from diasf January 27, 2025 08:58
@madryk madryk assigned diasf and unassigned rscipien Jan 27, 2025
<h:dataTable id="resultsTable" value="#{SearchIncludeFragment.searchResultsList}" var="result"
rendered="#{!(empty SearchIncludeFragment.searchResultsList)}">
<h:dataTable id="resultsTable" value="#{lastSearchValue.searchResultsList}" var="result"
rendered="#{!(empty lastSearchValue.searchResultsList)}">
<h:column>
<!--DATAVERSE CARDS-->
<div class="dataverseResult clearfix #{result.unpublishedState ? 'unpublished' : ''}" jsf:rendered="#{result.type eq searchObjectTypeEnum.DATAVERSES}">
<c:set var="dvUrl" value="/dataverse/#{result.dataverseAlias}"/>
<c:set var="dvUrlFinal" value="#{!SearchIncludeFragment.rootDv and !result.isInTree ? dvUrl : widgetWrapper.wrapURL(dvUrl)}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use lastSearchValue here as well? Wonder if we couldn't have changed the scope of SearchIncludeFragment from @RequestScoped to @ViewScoped instead? Could imaging that someone could make use of SearchIncludeFragment bean in the future without being aware of the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At some point in time SearchIncludeFragment was @ViewScoped. We have changed it to @RequestScoped to reduce down memory consumption in the application. Search pages are the most common page visited by bots and it seemed a really wasteful approach to keep in memory so much copies of basically dead data. Each page render even by the same bot can create separate ViewScope. Also taking into account that ViewScope can live as long as session lives and our session are very long it adds up. So I didn't want move backwards to the place like it was before.
I've also perceive existence of session scoped lastSearchValue as a threat to high memory consumption. But this was been already introduced (to be able to download csv with search results) so I used it also for this case. I didn't came up with more elegant solution.
Maybe if the component was not <h:dataTable> but simple <ui:repeat> we could use request scope . I didn't check it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fair enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh forgot about the other thing, so using SearchIncludeFragment.rootDv instead of lastSearchValue.rootDv is not an error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why but it does not fail. I would expect to NPE be thrown. Either way I will change it to lastSearchValue.rootDv.

@diasf diasf assigned madryk and unassigned diasf Jan 28, 2025
@madryk madryk merged commit 29017be into develop Jan 29, 2025
1 check passed
@madryk madryk deleted the feature/allow_for_button_action_inside_search_results branch January 29, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants