AO3-7079 Add series ID to work and bookmark blurbs #5535
Conversation
marcus8448
left a comment
marcus8448
left a comment
There was a problem hiding this comment.
Thanks for working on this! Just some notes on caching and minor test formatting issues.
| def css_classes_for_creation_blurb(creation) | ||
| return if creation.nil? | ||
|
|
||
| Rails.cache.fetch("#{creation.cache_key_with_version}/blurb_css_classes-v2") do |
There was a problem hiding this comment.
The cache key version should be updated
| if creation.nil? | ||
| "bookmark blurb group #{bookmarker_id_for_css_classes(bookmark)}" | ||
| else | ||
| Rails.cache.fetch("#{creation.cache_key_with_version}_#{bookmark.cache_key}/blurb_css_classes") do |
There was a problem hiding this comment.
This cache key should be updated as well
| end | ||
| end | ||
|
|
||
| context "when creation is work" do |
There was a problem hiding this comment.
| context "when creation is work" do | |
| context "when creation is Work" do |
Just for consistency
| end | ||
| end | ||
|
|
||
| context "When work belongs to a series" do |
There was a problem hiding this comment.
| context "When work belongs to a series" do | |
| context "when work belongs to a series" do |
| context "when creation is ExternalWork" do | ||
| let(:external_work) { create(:external_work) } | ||
|
|
||
| it "returns empty array for exteral work" do |
There was a problem hiding this comment.
Since the context states that it is an external work, it's not necessary to restate that in the it. (Same for the series test)
| let(:series_a) { create(:series) } | ||
| let(:series_b) { create(:series) } | ||
| let(:work_a) { series_a.works.first } | ||
| let(:work_b) { create(:work) } | ||
| let(:work_c) { create(:work, series: [series_a, series_b]) } | ||
|
|
||
| it "returns empty array for work with no series" do | ||
| result = helper.creation_series_ids_for_css_classes(work_b) | ||
| expect(result).to be_empty | ||
| end | ||
|
|
||
| it "returns array for work with series" do | ||
| result = helper.creation_series_ids_for_css_classes(work_a) | ||
| expect(result).to eq(["series-#{series_a.id}"]) | ||
| end | ||
|
|
||
| it "returns array for work with multiple series" do | ||
| result = helper.creation_series_ids_for_css_classes(work_c) | ||
| expect(result).to eq(["series-#{series_a.id}", "series-#{series_b.id}"]) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
It would be nice to make these tests each get their own context block (similar to the creator_ids_for_css_classes work tests just above) so it's easier to tell which work/series is being used in each test.
| end | ||
| end | ||
|
|
||
| context "when new user is added" do |
There was a problem hiding this comment.
It would also be good to get a test (similar to this one) for cache expiry when a series is added to a work.
2 participants
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-7079
Purpose
Add the series ID as a class to work and bookmark blurbs, formatted
series-1234for works that belong to one or more series.Testing Instructions
Testing instructions are in the JIRA ticket
Credit
Vemmy RM (she/her)