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

Insert existing doc into a new group #5

Merged
merged 6 commits into from
Jul 16, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 6 additions & 1 deletion lib/lexorank/rankable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,11 @@ def move_to(position)
if position == 0
[nil, collection.first]
else
current_position = collection.map(&:id).index(self.id)
Copy link
Owner

Choose a reason for hiding this comment

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

Not in scope of this PR but I didn't like this solution in first place because it's operating on the collection within ruby which is very costly in terms of performance.

However, previously it was only called if needed. Now it is executed every time if the position is not 0. For example running the scope benchmark, that creates 5000 records, will run around ~15x slower than before (at least on my machine).

Maybe we can change it to only run if no_rank? is false. This would preserve the original behavior. I still need to figure out a way around this thing entirely...

Copy link
Owner

Choose a reason for hiding this comment

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

Well I just thought about it for 5 minutes and I think I have a solution:

The only case where we want to use collection.offset(position).limit(2) is when the item has a rank and is in front of the collection. We can just avoid all the checks and use

collection.where.not(id: self.id).offset(position - 1).limit(2)

The tests tell me that this seems to work. I'm going to merge your PR, make the aforementioned change and am going to release a new version of the gem!

not_in_collection = current_position.nil?
# if item is currently in front of the index we just use position otherwise position - 1
# if the item has no rank we use position - 1
position -= 1 if !self.send(self.class.ranking_column) || collection.map(&:id).index(self.id) > position
position -= 1 if no_rank? || not_in_collection || current_position > position
collection.offset(position).limit(2)
end

Expand All @@ -85,6 +87,9 @@ def move_to_top!
move_to!(0)
end

def no_rank?
!self.send(self.class.ranking_column)
end
end

end
Expand Down
53 changes: 39 additions & 14 deletions test/group_by_test.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
require 'test_helper'

class GroupByTest < ActiveSupport::TestCase

should 'group paragraphs by page id and update accordingly' do
page_1 = Page.create
paragraphs_1 = create_sample_paragraphs(page_1)
Expand All @@ -20,11 +19,7 @@ class GroupByTest < ActiveSupport::TestCase
end

should 'resolve attribute names' do
class Paragraph1 < ActiveRecord::Base
self.table_name = "paragraphs"
rank!(group_by: :page_id)
end
assert_equal :page_id, Paragraph1.ranking_group_by
assert_equal :page_id, GroupedParagraph.ranking_group_by

class Paragraph2 < ActiveRecord::Base
self.table_name = "paragraphs"
Expand All @@ -45,17 +40,47 @@ class Paragraph3 < ActiveRecord::Base
assert_nil Paragraph3.ranking_group_by
end

def create_sample_paragraphs(page, count: 3)
paragraphs = []
count.times do
paragraphs << Paragraph.create(page: page)
describe 'moving to a different group' do
should 'insert into middle' do
page1, page2 = create_sample_pages(count: 2)
paragraph1, paragraph2, paragraph3 = create_sample_paragraphs(page1, clazz: GroupedParagraph)

new_paragraph = create_sample_paragraphs(page2, count: 1, clazz: GroupedParagraph).first

new_paragraph.page = page1
new_paragraph.move_to(2)
new_paragraph.save!

expected = [paragraph1, paragraph2, new_paragraph, paragraph3]
assert_equal expected, GroupedParagraph.where(page_id: page1.id).ranked
end

paragraphs.each_with_index do |paragraph, index|
paragraph.move_to!(index)
should 'insert at top' do
page1, page2 = create_sample_pages(count: 2)
paragraph1, paragraph2, paragraph3 = create_sample_paragraphs(page1, clazz: GroupedParagraph)

new_paragraph = create_sample_paragraphs(page2, count: 1, clazz: GroupedParagraph).first

new_paragraph.page = page1
new_paragraph.move_to(0)
new_paragraph.save!

expected = [new_paragraph, paragraph1, paragraph2, paragraph3]
assert_equal expected, GroupedParagraph.where(page_id: page1.id).ranked
end

paragraphs
end
should 'insert at the end' do
page1, page2 = create_sample_pages(count: 2)
paragraph1, paragraph2, paragraph3 = create_sample_paragraphs(page1, clazz: GroupedParagraph)

new_paragraph = create_sample_paragraphs(page2, count: 1, clazz: GroupedParagraph).first

new_paragraph.page = page1
new_paragraph.move_to(3)
new_paragraph.save!

expected = [paragraph1, paragraph2, paragraph3, new_paragraph]
assert_equal expected, GroupedParagraph.where(page_id: page1.id).ranked
end
end
end
5 changes: 5 additions & 0 deletions test/models/grouped_paragraph.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class GroupedParagraph < Paragraph
rank!(group_by: :page_id)
end


1 change: 0 additions & 1 deletion test/ranking_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,4 @@ class Page2 < ActiveRecord::Base
assert_equal('This rank should not be achievable using the Lexorank::Rankable module! ' +
'Please report to https://github.com/richardboehme/lexorank/issues! The supplied ranks were nil and "0". Please include those in the issue description.', error.message)
end

end
25 changes: 19 additions & 6 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

require 'models/page'
require 'models/paragraph'
require 'models/grouped_paragraph'

class Minitest::Test
include Shoulda::Context::DSL
Expand Down Expand Up @@ -51,16 +52,28 @@ def assert_not(condition)
assert !condition
end

def create_sample_pages(count: 3, clazz: Page)
pages = []
def create_sample_docs(count:, clazz:, create_with: {})
docs = []
count.times do
pages << clazz.create
docs << clazz.create(create_with)
end

pages.each_with_index do |page, index|
page.move_to!(index)
docs.each_with_index do |doc, index|
doc.move_to!(index)
end

pages
docs
end

def create_sample_pages(count: 3, clazz: Page)
create_sample_docs(count: count, clazz: clazz)
end

def create_sample_paragraphs(page, count: 3, clazz: Paragraph)
create_sample_docs(
count: count,
clazz: clazz,
create_with: { page: page },
)
end
end