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

V8 tracking configuration #2970

Merged
merged 6 commits into from
Feb 2, 2023
Merged

V8 tracking configuration #2970

merged 6 commits into from
Feb 2, 2023

Conversation

barmintor
Copy link
Contributor

This PR pulls the breaking configuration changes out of #2954 without the new features. This would allow us to pull the sessionStorage changes in to Blacklight 8.x in a feature release. This PR:

  • moves all render? logic for SearchContextComponent into the component
  • adds an action to Blacklight::Searchable that returns the prev/next document IDs for a given offset and search
  • changes track_search_session in Blacklight::Configuration from a boolean to a configuration object allowing component configuration
  • extracts the other part of searchcontext presentation - item pagination - into a component as well
  • renames Blacklight::SearchContextComponent to Blacklight::SearchContext::ServerItemPaginationComponent

@barmintor barmintor added this to the 8.0 milestone Jan 31, 2023
@barmintor barmintor force-pushed the v8-tracking-configuration branch from 010dac7 to e1d298f Compare January 31, 2023 22:03
@jcoyne
Copy link
Member

jcoyne commented Feb 1, 2023

@barmintor would you be able to do something with the "squash" commit?

@barmintor
Copy link
Contributor Author

😆 yes, absolutely

…search_session

- allow configuration of components for applied parameters and item pagination
- only build server-side search tracking data if using server-side tracking
- extract applied_params html into a component
@barmintor barmintor force-pushed the v8-tracking-configuration branch from e1d298f to e45aafa Compare February 1, 2023 18:27
@barmintor barmintor force-pushed the v8-tracking-configuration branch from e45aafa to 9bd3ecb Compare February 1, 2023 18:38
Copy link
Member

@jcoyne jcoyne left a comment

Choose a reason for hiding this comment

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

Should Blacklight::SearchContextComponent be deprecated in this PR?

Copy link
Contributor

@carolyncole carolyncole left a comment

Choose a reason for hiding this comment

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

Looks great! Just noticed two inconstancies..

end

def search_id
controller.current_search_session&.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this is not search_session and you are reaching into the controller?

Copy link
Member

@dkinzer dkinzer Feb 2, 2023

Choose a reason for hiding this comment

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

Yeah, I agree this seems like a mistake. The only scenarios I can think of where it might make sense is where @search_session is nil or if @search_session would ever differ from controller.current_search_sesssion. But neither of those two scenarios makes sense to me either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, this is a little complicated: @search_session is a Hash representation of a search. controller.current_search_session is an instance of the Search model. I think we can rely on @search_session['id'] for this value, which should be loaded in a before_action added to controllers when they include Blacklight::Catalog. But it relies on more assumptions than this implementation.

Again, this was not introduced in this PR. This is the existing behavior in main of a component that is renamed in this PR.

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 think it might be good to separately ticket this change.

Copy link
Member

Choose a reason for hiding this comment

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

@barmintor I agree with you about a separate ticket vs changing it in this PR. The implications of an instance variables vs a hash representation may be very significant in a way that is not yet clear. I keep wondering if there is a context in which these two different representation are different and it does matter which one is used at this point. That would be bad design but still an open question.

@barmintor
Copy link
Contributor Author

@jcoyne I think the deprecation of Blacklight::SearchContextComponent should be backported to 7.x, and it should just be renamed in 8.x.

@dkinzer
Copy link
Member

dkinzer commented Feb 2, 2023

Hey following up on my reservations from the last committer meeting. I tried to cherry pick this commit onto the 7.33.0 branch to test against my current app. But unfortunately I get the error

 render(Blacklight::SearchContextComponent.new(search_context: @search_context, search_session: search_session))

on my app when I try to render the show page. Looks like we are overriding a template that specifically references that removed class. But I don't want the fact that I can't test this in my v7 app to block this any further so I'm happy to have it merged and just cross that potential bridge if/when the time comes when we upgrade the app to v8.

@barmintor
Copy link
Contributor Author

@dkinzer that component is renamed in this PR to Blacklight::SearchContext::ServerItemPaginationComponent

Co-authored-by: carolyncole <1599081+carolyncole@users.noreply.github.com>
@dkinzer
Copy link
Member

dkinzer commented Feb 2, 2023

@dkinzer that component is renamed in this PR to Blacklight::SearchContext::ServerItemPaginationComponent

Thanks. I'll try that.

@dkinzer
Copy link
Member

dkinzer commented Feb 2, 2023

@barmintor ... with the change you suggest I then get the following error:

Showing /Users/dkinzer/projects/tul_cob/app/views/catalog/_previous_next_doc.html.erb where line #13 raised:

private method `current_search_session' called for #<CatalogController:0x00000000034ad0>

        controller.current_search_session&.id
                  ^^^^^^^^^^^^^^^^^^^^^^^

@barmintor
Copy link
Contributor Author

@dkinzer seems likely to be fixed by the change that @carolyncole requested

@barmintor
Copy link
Contributor Author

@dkinzer you might also try changing that call from controller.current_search_session to helpers.current_search_session in your branch

@dkinzer
Copy link
Member

dkinzer commented Feb 2, 2023

@barmintor when I use either methods suggested for the fix I get a new error:

NoMethodError: private method `session_tracking_params' called for #<ActionView::Base:0x0000000005a4d8>

If I try to fix that by calling the private method via a send message I get:

NoMethodError: undefined method `id' for [nil, 0]:Array

    path = session_tracking_path(document, per_page: params.fetch(:per_page, search_session['per_page']), counter: counter, search_id: current_search_session.try(:id), document_id: document&.id)

Copy link
Contributor

@carolyncole carolyncole left a comment

Choose a reason for hiding this comment

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

Based on the discussion I'm approving this PR.

@carolyncole carolyncole merged commit 4f610ca into main Feb 2, 2023
@carolyncole carolyncole deleted the v8-tracking-configuration branch February 2, 2023 18:42
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.

4 participants