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

Resolve "Adding bookmark without bookmark counter raises an error" #3215

Conversation

mamrey
Copy link
Contributor

@mamrey mamrey commented Jun 25, 2024

Closes #3214

@jcoyne
Copy link
Member

jcoyne commented Jun 25, 2024

@mamrey thank you.

@mamrey
Copy link
Contributor Author

mamrey commented Jun 25, 2024

There was a rubocop issue that should be resolved now :)

@cgalarza
Copy link
Contributor

cgalarza commented Jul 2, 2024

We are very interested in getting this fix merged and released so we can update our new Catalog to the newest version of blacklight. Could a maintainer approve the workflow for this MR? I am happy to become a maintainer if that would be helpful!

@jcoyne
Copy link
Member

jcoyne commented Jul 16, 2024

Can this be rebased on main? I believe the problem with Ruby 3.2 builds is resolved there.

@cgalarza
Copy link
Contributor

Amrey is away on vacation and I don't have access to his fork. He's back on Monday, but if there's another way I could do it, I am happy to.

@jcoyne
Copy link
Member

jcoyne commented Jul 16, 2024

@cgalarza you should be able to git cherry-pick his commit into your own branch.

…it_from_raising_error_when_bookmark_counter_is_not_rendered
@cgalarza
Copy link
Contributor

I ran the pipeline in my local fork and there's still an error. I'll have to look at it tomorrow.

cgalarza#2

@cgalarza
Copy link
Contributor

cgalarza commented Jul 17, 2024

@jcoyne I am wondering if you have any advice on how to fix this error. It seems that capybara is not finding the 'Bookmarks' link because of the badge that's included in the link. The test works if I change the problematic line to:

  • click_link 'Bookmarks'
  • click_on :bookmarks_nav
  • click_on 'Bookmarks 0'

The test also passes if I add the :js tag to the test.

@cgalarza
Copy link
Contributor

Hi @jcoyne, @mamrey and I resolved the testing issue. We've pushed up a change that we believe fixes the problem and have merged in the last changes from main.

@tampakis tampakis merged commit 7f3f38f into projectblacklight:main Jul 22, 2024
12 checks passed
@jcoyne
Copy link
Member

jcoyne commented Aug 1, 2024

@mamrey @cgalarza I've been having a bit of trouble in the BL codebase recently and It keeps coming back to the test from this PR. I'm especially confused about the stub for render_bookmarks_control? as this isn't a controller method:

irb(main):001> CatalogController.render_bookmarks_control?
(irb):1:in `<main>': undefined method `render_bookmarks_control?' for class CatalogController (NoMethodError)

CatalogController.render_bookmarks_control?
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^

When that method on the CatalogController instance is false I don't believe it displays the "bookmark" control, which the test would depend on. Can you take a look?

@jcoyne
Copy link
Member

jcoyne commented Aug 1, 2024

I removed this test in #3245 to get the build working.

@mamrey
Copy link
Contributor Author

mamrey commented Aug 2, 2024

@jcoyne

Thank you for working on this. Yes, the test depends on the bookmark control not being rendered. The initial spec attempted to achieve this by adding the appropriate blacklight config. When looking into the build failures we found this test that we took inspiration from. I am okay with removing the test as you've done.

@jcoyne
Copy link
Member

jcoyne commented Aug 2, 2024

@mamrey the difference is that in

allow(controller).to receive(:render_bookmarks_control?).and_return(true)
the method is stubbed on the controller instance and in this PR it's stubbed on the class (there is no method on the class).

@mamrey mamrey mentioned this pull request Aug 8, 2024
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.

Adding bookmark without bookmark counter raises an error
4 participants