AO3-7214 AO3-7215 Bookmark Javascript Improvement #5499
Conversation
Changed id to class so that every button that can do one feature behaves the same and changed the organisation of the jquery
Since the new tests have been moved to their new feature file to test javascript feature these are not needed
|
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. |
|
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
changed the title
Changed the PR name accordingly |
sarken
left a comment
sarken
left a comment
There was a problem hiding this comment.
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.
github-actions
bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
|
I pushed changes to the gemfile.lock by accident. Disregard the Gem Updates tag |
| edit_bookmark_path(current_user_bookmark), | ||
| id: "bookmark_form_trigger_for_#{bookmarkable.id}", | ||
| remote: true %> | ||
| <%= link_to t(".saved"), edit_bookmark_path(current_user_bookmark), data: { path: edit_bookmark_path(current_user_bookmark) }, class: "bookmark_form_trigger_for_#{bookmarkable.id}", remote: true %> |
There was a problem hiding this comment.
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.)
github-actions
bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
|
Gonna put this to draft while I add the rest of the test coverage. |
|
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? |
|
I shall put this as ready for review as I do not understand why the step fails in the test. |
sarken
left a comment
sarken
left a comment
There was a problem hiding this comment.
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.
| Then "the bookmark form should be open in the bookmarkable blurb for {string}" do |title| | ||
| work_id = Work.find_by(title: title).id | ||
| within("#bookmark_#{work_id}") do | ||
| step %{I should see "save a bookmark!"} | ||
| step %{I should not see "Saved"} | ||
| step %{I should not see "Edit"} | ||
| end | ||
| end | ||
|
|
||
| Then "the bookmark form should be open in the blurb for {word}'s bookmark of {string}" do |login, title| | ||
| user = User.find_by(login: login) | ||
| work = Work.find_by(title: title) | ||
| bookmark_id = user.bookmarks.find_by(bookmarkable: work).id | ||
| within("#bookmark_#{bookmark_id}") do | ||
| step %{I should see "save a bookmark!"} | ||
| step %{I should not see "Edit"} | ||
| end | ||
| end |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Hmm, given what is the scope of the test, I feel like the first one is more telling.
| Feature: Edit bookmarks with javascript enabled | ||
| In order to have a good user experience | ||
| As a humble user | ||
| I want to be able to edit bookmarks with javascript enabled |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Given how consistently the I bookmark the work X step seemed to be, I decided to add a new step definition. I could combine it with the previous one if that is simpler (as they are quite similar)
github-actions
bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
4 participants
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