AO3-7214 AO3-7215 Bookmark Javascript Improvement

[ReverM]

Issue

https://otwarchive.atlassian.net/browse/AO3-7214
https://otwarchive.atlassian.net/browse/AO3-7215

Purpose

This adresses both tickets 7214 and 7215 by making all buttons that could edit a bookmark disappear when the form is open, by allowing a closed form to be reopened (7215) and by fixing the opening and closing of multiple forms at the same time.
7214 was fixed by removing the variables and jquerying the appropriate component each time the form is closed or reopened.
7215 was fixed by changing the href of the opening buttons as part of the closing event, as well as changing the id to a class so every element is affected at the same time.

Credit

Danaël / Rever ( they / he )

Jira account goes by Danaël Villeneuve

[ReverM]

Appologies if having multiple ticket in one PR is bad etiquette. I decided to put both in the same PR, as they were extremely closely related, and would likely be waiting on each other if done separately.

[slavalamp]

yeah, putting two tickets in one pr is fine when it's exact the same fix for both

though the pr name should begin like this: "AO3-7214 AO3-7215"

[ReverM]

though the pr name should begin like this: "AO3-7214 AO3-7215"

Changed the PR name accordingly

[sarken]

Thanks for working on this! In addition to the question I left on the JavaScript file, I think you also need to changeid to class for the buttons in the bookmarkable_blurb, which is used for pages where we show multiple bookmarks under a single item, e.g., a tag's bookmark listing.

[ReverM]

I pushed changes to the gemfile.lock by accident. Disregard the Gem Updates tag

[ReverM]

Gonna put this to draft while I add the rest of the test coverage.

[ReverM]

Some of the steps in the test are very inconsistent. I'm unsure what could be the cause. Is cucumber not the best friend with javascript? They overall seem very flaky. What could be explaining this?

[ReverM]

I shall put this as ready for review as I do not understand why the step fails in the test.

[sarken]

Our JavaScript tests do tend to be flaky, unfortunately. If these continue to be a problem, we can update our list of known test failures to mention features/bookmarks when they're merged.

[sarken]

It would probably be a good idea to change this section to talk about both creating and editing now that we have some tests for both.

[ReverM]

I'm unsure where exactly to put this, but for some reason, clicking on the bookmark link with javascript enabled doesnt work on github. It's one that's flaky on my end, but it's consistently not working on github. Is there an alternate way to setup pre-existing bookmarks with cucumber without passing by steps?

[sarken]

I had the exact opposite problem: I was having consistent local failures, but it was working more often than not on GitHub Actions. The second scenario still doesn't pass locally for me even with the changes here, but we can live with that.

[ReverM]

What step exactly fails?

[sarken]

Then I should see "Bookmark" within "#main .navigation" in features/bookmarks/bookmark_javascript.feature:13

[ReverM]

I was looking at the documentation of Capybara, and it seemed to say to use js: true to switch to the Capybara.javascript_driver. This is not something we seem to be doing, so could this explain the flakiness of the javascript tests? https://github.com/teamcapybara/capybara#using-capybara-with-rspec

We currently have the hook before "@javascript" do which changes the host and set the javascript variable to true (so that the website renders with javascript), but no driver change.

[sarken]

Unfortunately, no. That section is about RSpec, but these are Cucumber tests, which are discussed in the previous section. Using the @javascript tag in our tests automatically switches to the JavaScript driver, which is defined in our config:

otwarchive/features/support/capybara.rb

Line 82 in be7aee3

Capybara.javascript_driver = :selenium_chrome_headless

[ReverM]

Ah I see

[sarken]

Could you address the comment at the very top of this thread (the one about updating the description)? Then this will be ready to merge! 🎉

[sarken]

Just a few little things left, most of which are from Reviewdog! Thank you so much for working on this; I'm sorry it turned out to be so much more complicated than it looked.

[sarken]

I had the exact opposite problem: I was having consistent local failures, but it was working more often than not on GitHub Actions. The second scenario still doesn't pass locally for me even with the changes here, but we can live with that.

[sarken]

🎉