Back to browse results feature

[rominail]

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:

• Merge and reconcile Introduce GetServiceTrait. #3843

[demiankatz]

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.

[demiankatz]

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.

[demiankatz]

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!

[demiankatz]

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?

[rominail]

@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]

@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]

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

[demiankatz]

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.

[rominail]

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

[EreMaijala]

@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)

[demiankatz]

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

[rominail]

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]

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.

[demiankatz]

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

[demiankatz]

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

[rominail]

I changed it, is it better now?

[demiankatz]

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?

[rominail]

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

[demiankatz]

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.

[demiankatz]

@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. :-)

[rominail]

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

[demiankatz]

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.

[rominail]

Any more thoughts? What should I do?

[demiankatz]

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.

[rominail]

@demiankatz @EreMaijala Any more thoughts on this?

[demiankatz]

@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.