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

Back to browse results feature #3826

Open
wants to merge 35 commits into
base: dev
Choose a base branch
from

Conversation

rominail
Copy link
Contributor

@rominail rominail commented Jul 15, 2024

I'm not sure about :

  • where (and if) to put the css
  • The package name
  • If the translation place and way to do it is good
  • the AlphaBrowse helper has the name Authentication, which I marked with a question mark, it seems to me it's a copy paste error, however I'm not sure neither what the name should be

TODO:

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for sharing this, @rominail. I haven't gone through all of the code in detail yet, but I had a few initial thoughts and suggestions that might be worth discussing before getting too deep into the work. Let me know what you think! I think what you're adding here is a useful improvement, but I want to get us on the same page about the approach. It seems to me like some of this could be simplified, but I'm open to being persuaded otherwise if you disagree.

module/VuFind/src/VuFind/Controller/AbstractRecord.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Controller/AbstractSearch.php Outdated Show resolved Hide resolved
languages/en.ini Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Search/Base/Params.php Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for the progress, @rominail. I have a few more questions and comments. I confess that I still haven't had a chance to try this hands-on yet, and I probably need to give it a more thorough comprehensive read-through... but hopefully these suggestions will help in the meantime.

languages/en.ini Outdated Show resolved Hide resolved
themes/bootstrap5/templates/header.phtml Outdated Show resolved Hide resolved
themes/bootstrap3/scss/components/search.scss Outdated Show resolved Hide resolved
themes/bootstrap5/templates/header.phtml Outdated Show resolved Hide resolved
themes/root/templates/search/backtoorigin.phtml Outdated Show resolved Hide resolved
themes/root/templates/search/backtoorigin.phtml Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for the continued progress on this -- seems to be shaping up nicely. Just one terminology adjustment I'd suggest (see below). Next week, when I'm back in the office, I'll give this a more thorough test and review to see if there are any larger issues to address!

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this, @rominail. I took a deeper look at the code and left a few more comments (all minor -- your basic design seems sound).

I tried to do some hands-on testing, though, and I couldn't get this to work. My expectation was that if I performed an alphabrowse search and clicked on one of the results, I would then see a back to browse link in subsequent pages. This did not occur -- the URL contained extra GET parameters, but I didn't see any new links anywhere. Is this working on your end? Am I doing something wrong?

module/VuFind/src/VuFind/Controller/AbstractBase.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Controller/AbstractRecord.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Controller/AbstractRecord.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/View/Helper/Root/AlphaBrowse.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/View/Helper/Root/SearchMemory.php Outdated Show resolved Hide resolved
themes/bootstrap3/less/components/record.less Outdated Show resolved Hide resolved
themes/bootstrap3/templates/header.phtml Outdated Show resolved Hide resolved
@rominail
Copy link
Contributor Author

@demiankatz The code is working on my end, the feature is what you are expecting; do you have the file backtoorigin.phtml? In the first version I named it incorrectly with the .php extension and it was not working, I don't know if it could be the cause, any logs otherwise?

@demiankatz
Copy link
Member

@demiankatz The code is working on my end, the feature is what you are expecting; do you have the file backtoorigin.phtml? In the first version I named it incorrectly with the .php extension and it was not working, I don't know if it could be the cause, any logs otherwise?

I must have tested before this was fixed. It is working now -- thanks!

@rominail
Copy link
Contributor Author

Cool! Anything else? I set the PR as ready to review (removed the draft state)

@rominail rominail marked this pull request as ready for review July 30, 2024 15:04
@demiankatz
Copy link
Member

Cool! Anything else? I set the PR as ready to review (removed the draft state)

I'm interested in getting some more opinions on this, but I think it's nearly done. I'll add it to the 10.1 milestone and make sure it gets discussed on next week's Community Call in case that yields additional feedback.

@demiankatz demiankatz added this to the 10.1 milestone Jul 30, 2024
themes/bootprint3/less/search.less Outdated Show resolved Hide resolved
themes/bootprint3/scss/search.scss Outdated Show resolved Hide resolved
themes/bootstrap3/templates/search/backtoorigin.phtml Outdated Show resolved Hide resolved
@rominail
Copy link
Contributor Author

@demiankatz @EreMaijala
I made all the suggestions you offered, let me know if there is anything else

@demiankatz demiankatz requested a review from EreMaijala August 12, 2024 19:41
@EreMaijala
Copy link
Contributor

@demiankatz Since discussions in review comment threads are really hard to find after being marked resolved, here's the the one you had trouble finding: #3826 (comment)

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @rominail -- see below for one small proposed cleanup action, and a suggestion regarding the breadcrumb vs. stand-alone control situation.

themes/bootstrap3/templates/header.phtml Outdated Show resolved Hide resolved
themes/bootstrap3/templates/layout/layout.phtml Outdated Show resolved Hide resolved
@rominail
Copy link
Contributor Author

Particularly strange behavior: when I click on a browse result, I sometimes end up on the record page with the extra origin parameters, but other times those parameters are missing. Whether or not the parameters are defined, I never see a back to browse link.

Also, something else to consider: sometimes a browse link leads to a single record, and other times it leads to a search result set showing multiple records. I hadn't previously considered the "search result set" scenario, but we'll want to be sure that behaves reasonably as well.

You end up on the record page if there is only one result in the alphabrowse, if there are at least 2 you get to the search results; this case scenario is already taken care of

@demiankatz
Copy link
Member

Particularly strange behavior: when I click on a browse result, I sometimes end up on the record page with the extra origin parameters, but other times those parameters are missing. Whether or not the parameters are defined, I never see a back to browse link.
Also, something else to consider: sometimes a browse link leads to a single record, and other times it leads to a search result set showing multiple records. I hadn't previously considered the "search result set" scenario, but we'll want to be sure that behaves reasonably as well.

You end up on the record page if there is only one result in the alphabrowse, if there are at least 2 you get to the search results; this case scenario is already taken care of

I understand why sometimes you get the record and sometimes you get search results... but what was strange was that in some situations, the origin parameters were missing in the "single record" case, and in other situations they were present. I couldn't reproduce the problem today after you made your fixes, so maybe the issue was related to something you've subsequently corrected.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for the ongoing progress, @rominail -- I gave this another more thorough review and noticed a few more small issues.

*/
public const SEARCH_SOURCE_DISPLAY_PARAM = 'AB-sd';

/**
Copy link
Member

Choose a reason for hiding this comment

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

The description is the same for SEARCH_SOURCE_DISPLAY_PARAM and SEARCH_SOURCE_PARAM. Can you clarify the difference between these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it, is it better now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that makes more sense... though I wonder if it would be better to simply look up the display name of the source in the alphabrowse origin class, instead of passing it visibly through the URL. I realize this would be non-trivial (we'd need to inject the config through the factory, and perhaps create a trait or helper class to make the existing getTypes method available in more places than just the AlphaBrowseController). Do you think it might be worth the effort in order to make things slightly more seamless? Or is that too much work/complexity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do it at first after, like you, realizing it's heavy, however I'm happy to do it if you feel it would be good
What we might do, if it makes sense for you, it's to make it whenever we implement another origin needing a code improvement

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I could live with performing this simplification as a future follow-up, after getting the basic system in place.

In the meantime, I've just pushed up a small commit to make slight adjustments to the comment on the display property.

I'll also follow up with @EreMaijala and see if he has any further thoughts before we move forward with this.

Copy link
Member

Choose a reason for hiding this comment

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

@rominail, just a note to say that this hasn't been forgotten about. @EreMaijala and I had a long discussion about this PR and would like to find a way to achieve the same effect with fewer query parameters and less explicit plumbing in the controllers, to keep things simpler and more decoupled. I think there are at least two possible alternative approaches that might be improvements:

1.) There has been a long-standing ticket (VUFIND-1652) to create a mechanism for persisting session-specific data in the database. We could probably use this to persist a serialized SearchOrigin object, and then we could pass a simple identifier in the query and use it to retrieve the full object from the database. I think this would make things look a lot cleaner, though it requires us to build that mechanism before we could implement it.

2.) There's also a possibility of refactoring code so that browsing is treated as a subset of searching instead of a distinct activity. We could then use the existing "sid" to keep track of browse activities, and add the ability to chain searches together for breadcrumb purposes. This would be a totally different solution to the problem and would require a lot of code changes, but it holds some theoretical appeal at least.

Unfortunately, both of these theoretical approaches require some groundwork would prevent this PR to get finished right away -- so the next question is whether you're in a hurry to get this resolved for your local needs. If you don't mind waiting, I'd be interested to hear if you have preferences, and then we can start working on strategy to get the necessary prerequisites in place. If you're in a hurry, though, maybe we can find a different compromise. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On our sides we are in no rush, the modification has already been made on our instance
I don't really have strong feeling
Regarding the sessions and / or storing anything in DB, in case of a bot crawling (not following rules) (which happens regularly) it loads the servers significantly

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @rominail -- since you're not in a hurry and some more thought is needed here, I've moved this to the 11.0 milestone for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any more thoughts? What should I do?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to take any action right now. I'm hoping that @EreMaijala will have some more suggestions eventually, but since we're currently more focused on finishing up release 10.1, this will probably be on hold for a bit until work on 11.0 intensifies.

module/VuFind/src/VuFind/Search/Minified.php Outdated Show resolved Hide resolved
@demiankatz demiankatz added the new language strings adds new text in need of translation label Sep 3, 2024
@demiankatz demiankatz modified the milestones: 10.1, 11.0 Sep 6, 2024
@rominail
Copy link
Contributor Author

@demiankatz @EreMaijala Any more thoughts on this?

@demiankatz
Copy link
Member

@rominail, @EreMaijala and I have had some conversations about this and have some ideas for streamlining it -- but at the moment our time is fully occupied with fixing bugs and dealing with urgent technical debt caused by Laminas component abandonment. This is definitely still on the to-do list (the 11.0 milestone will ensure that we don't forget about it), but we unfortunately haven't had time to give it the attention it deserves quite yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement new language strings adds new text in need of translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants