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

Update site updates when exercises change and add wip guard #2009

Merged
merged 4 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions app/commands/concept_exercise/create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ def call
track: track,
**attributes
).tap do |exercise|
SiteUpdates::NewExerciseUpdate.create!(
exercise: exercise,
track: track
)
SiteUpdates::ProcessNewExerciseUpdate.(exercise)
end
rescue ActiveRecord::RecordNotUnique
ConceptExercise.find_by!(uuid: uuid, track: track)
Expand Down
1 change: 1 addition & 0 deletions app/commands/git/sync_concept_exercise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def call

SyncExerciseAuthors.(exercise)
SyncExerciseContributors.(exercise)
SiteUpdates::ProcessNewExerciseUpdate.(exercise)
end

private
Expand Down
1 change: 1 addition & 0 deletions app/commands/git/sync_practice_exercise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def call

SyncExerciseAuthors.(exercise)
SyncExerciseContributors.(exercise)
SiteUpdates::ProcessNewExerciseUpdate.(exercise)
end

private
Expand Down
5 changes: 1 addition & 4 deletions app/commands/practice_exercise/create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ def call
track: track,
**attributes
).tap do |exercise|
SiteUpdates::NewExerciseUpdate.create!(
exercise: exercise,
track: track
)
SiteUpdates::ProcessNewExerciseUpdate.(exercise)
end
rescue ActiveRecord::RecordNotUnique
PracticeExercise.find_by!(uuid: uuid, track: track)
Expand Down
40 changes: 40 additions & 0 deletions app/commands/site_updates/process_new_exercise_update.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
module SiteUpdates
class ProcessNewExerciseUpdate
include Mandate

initialize_with :exercise

def call
if exercise.wip?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in the unlikely case that a new exercise is created with the deprecated status?

destroy!
else
begin
create!
rescue ActiveRecord::RecordNotUnique
update!
end
end
end

def destroy!
SiteUpdates::NewExerciseUpdate.where(
exercise: exercise,
track: exercise.track
).destroy_all
end

def create!
SiteUpdates::NewExerciseUpdate.create!(
exercise: exercise,
track: exercise.track
)
end

def update!
SiteUpdates::NewExerciseUpdate.find_by!(
exercise: exercise,
track: exercise.track
).regenerate_rendering_data!
end
end
end
4 changes: 4 additions & 0 deletions app/models/concerns/is_paramaterised_sti.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ def params(*keys)
end
end

def regenerate_rendering_data!
update!(rendering_data_cache: {})
end

def rendering_data
data = rendering_data_cache
if data.blank?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddIndexToUniquenessKeyOnSiteUpdates < ActiveRecord::Migration[6.1]
def change
add_index :site_updates, :uniqueness_key, unique: true
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2021_09_25_111900) do
ActiveRecord::Schema.define(version: 2021_10_01_151540) do

create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
t.string "name", null: false
Expand Down Expand Up @@ -449,6 +449,7 @@
t.index ["exercise_id"], name: "index_site_updates_on_exercise_id"
t.index ["pull_request_id"], name: "index_site_updates_on_pull_request_id"
t.index ["track_id"], name: "index_site_updates_on_track_id"
t.index ["uniqueness_key"], name: "index_site_updates_on_uniqueness_key", unique: true
end

create_table "solution_comments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
Expand Down
11 changes: 4 additions & 7 deletions test/commands/concept_exercise/create_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,12 @@ class ConceptExercise::CreateTest < ActiveSupport::TestCase
end

test "creates site_update" do
track = create :track
exercise = ConceptExercise::Create.(
SiteUpdates::ProcessNewExerciseUpdate.expects(:call)

ConceptExercise::Create.(
SecureRandom.uuid,
track,
create(:track),
build(:concept_exercise).attributes.symbolize_keys
)

update = SiteUpdate.first
assert_equal exercise, update.exercise
assert_equal track, update.track
end
end
7 changes: 7 additions & 0 deletions test/commands/git/sync_concept_exercise_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -352,4 +352,11 @@ class Git::SyncConceptExerciseTest < ActiveSupport::TestCase

assert_equal 'log-levels', exercise.slug
end

test "updates site_update" do
exercise = create :concept_exercise, uuid: 'f4f7de13-a9ee-4251-8796-006ed85b3f70', slug: 'logs', git_sha: "c75486b75db8012646b0e1c667cb1db47ff5a9d5", synced_to_git_sha: "c75486b75db8012646b0e1c667cb1db47ff5a9d5" # rubocop:disable Layout/LineLength
SiteUpdates::ProcessNewExerciseUpdate.expects(:call).with(exercise)

Git::SyncConceptExercise.(exercise, force_sync: true)
end
end
7 changes: 7 additions & 0 deletions test/commands/git/sync_practice_exercise_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -333,4 +333,11 @@ class Git::SyncPracticeExerciseTest < ActiveSupport::TestCase

assert exercise.reload.has_test_runner?
end

test "updates site_update" do
exercise = create :practice_exercise, uuid: '185b964c-1ec1-4d60-b9b9-fa20b9f57b4a'
SiteUpdates::ProcessNewExerciseUpdate.expects(:call).with(exercise)

Git::SyncPracticeExercise.(exercise, force_sync: true)
end
end
11 changes: 4 additions & 7 deletions test/commands/practice_exercise/create_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,12 @@ class PracticeExercise::CreateTest < ActiveSupport::TestCase
end

test "creates site_update" do
track = create :track
exercise = PracticeExercise::Create.(
SiteUpdates::ProcessNewExerciseUpdate.expects(:call)

PracticeExercise::Create.(
SecureRandom.uuid,
track,
create(:track),
build(:practice_exercise).attributes.symbolize_keys
)

update = SiteUpdate.first
assert_equal exercise, update.exercise
assert_equal track, update.track
end
end
40 changes: 40 additions & 0 deletions test/commands/site_updates/process_new_exercise_update_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
require "test_helper"

class ConceptExercise::CreateTest < ActiveSupport::TestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ConceptExercise::CreateTest < ActiveSupport::TestCase
class SiteUpdates::ProcessNewExerciseUpdateTest < ActiveSupport::TestCase

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I reckon loads of flakey tests happen because of this bug.

test "no-op if wip and not present" do
exercise = create :concept_exercise, status: :wip

SiteUpdates::ProcessNewExerciseUpdate.(exercise)

refute SiteUpdate.exists?
end

test "deletes if wip and was present" do
exercise = create :concept_exercise, status: :wip

create :new_exercise_site_update, exercise: exercise

SiteUpdates::ProcessNewExerciseUpdate.(exercise)

refute SiteUpdate.exists?
end

test "creates if not wip" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider adding explicit tests for beta and active.

exercise = create :concept_exercise

SiteUpdates::ProcessNewExerciseUpdate.(exercise)

assert SiteUpdate.where(exercise: exercise).exists?
end

test "updates if not wip and exists" do
exercise = create :concept_exercise
update = create :new_exercise_site_update, exercise: exercise
update.update_column(:rendering_data_cache, { "foo" => "bar" })
assert_equal({ "foo" => "bar" }, update.rendering_data_cache) # Sanity

SiteUpdates::ProcessNewExerciseUpdate.(exercise)

assert_equal JSON.parse(update.cacheable_rendering_data.to_json), update.reload.rendering_data_cache
end
end