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

[sarken]

I think the multiline format was a little easier to read, if you could keep that here. (Reviewdog might have some opinions about the indenting, though. It usually takes me a few tries to figure out exactly what it wants.)

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

We're also using these steps to test the Save button. When we're testing Save, we should be checking I should not see "Save" (and we don't really need to check for an Edit button, either). What if we refactored them a bit to specify whether we're opening an edit bookmark or new bookmark form, e.g.,

"the {word} bookmark form should be open in the bookmarkable blurb for {string}" do |action, title|

Then we could adjust the test based on the value of action:

within("#bookmark_#{work_id}") do
  step %{I should see "save a bookmark!"}
  if action == "edit"
    step %{I should not see "Saved"}
    step %{I should not see "Edit"}
  elsif action == "new"
    step %{I should not see "Save"}
  end
end

Of course, that's a lot of code to repeat between two steps. That's not the end of the world with tests, in my opinion, so it's okay to stop here. However, we could go a step further and pull it out into its own step, like:

Then "the {word} bookmark form should be open" do |action|
  step %{I should see "save a bookmark!"}
  if action == "edit"
  [...]
end

And then we'd only need to reference that in our within blocks, e.g.:

within("#bookmark_#{work_id}") do
  step %{the #{action} bookmark form should be open}
end

[sarken]

I think we can remove this step definition and "{word}'s bookmark form should be closed in the bookmarkable blurb for {string}" in favor of adjusting the two basic the bookmark form should be closed step definitions. We can figure out if the bookmark creator and the currently-logged-in user are the same just by looking at the HTML.

The first thing to do would be exactly what we did for the earlier "the bookmark form should be open" steps: make it so we can specify whether we're looking at a new or edit form, and then adjust the expectations based on that:

  step %{I should not see "save a bookmark!"}
  if action == "edit"
    step %{I should see "Saved"}
    step %{I should see "Edit"}
  elsif action == "new"
    step %{I should see "Save"}
  end

The test failures that happen after that should just be for the special case you were addressing in "{word}'s bookmark form should be closed in the blurb for {word}'s bookmark of {string}": the case where the bookmark blurb we're checking belongs to the currently-logged-in user.

We can handle that by using has_selector? to check if our bookmark blurb contains the own class:

  blurb_id = "#bookmark_#{bookmark_id}"
  # Bookmarks owned by the current user don't have Save or Saved links, only Edit
  if find(blurb_id).has_selector?(".own")
    step %{I should see "Edit" within "#{blurb_id}"}
  else
    within(blurb_id) do
      step %{the #{action} bookmark form should be closed}
    end
  end

[sarken]

Can you add a newline at the end of the file here? (It might be possible to configure your editor to do this automatically, depending on which you use.)

[ReverM]

I use VSCode, do you know how to set that up?

[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?