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.

Sign up for GitHub

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

feat: updated api to allow ordering on endorsed status #425

Merged
merged 3 commits into from
Mar 25, 2024
Merged

feat: updated api to allow ordering on endorsed status #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9db339c
feat: updated api to allow ordering on endorsed status
ayesha-waris Feb 6, 2024
00a8a04
test: fixed test cases
ayesha-waris Feb 29, 2024
9a1d9cc
test: fixed failing test case
ayesha-waris Mar 1, 2024
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: 4 additions & 1 deletion api/comment_threads.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
error 404, [t(:requested_object_not_found)].to_json
end

merge_question_type_responses = value_to_boolean(params["merge_question_type_responses"])

# user is required to return user-specific fields, such as "read" (even if bool_mark_as_read is False)
if params["user_id"]
user = User.only([:id, :username, :read_states]).find_by(external_id: params["user_id"])
Expand Down Expand Up @@ -63,7 +65,8 @@
error 400, [t(:param_exceeds_limit, :param => resp_limit, :limit => size_limit)].to_json
end
presenter.to_hash(
bool_with_responses, resp_skip, resp_limit, bool_recursive, bool_flagged_comments, bool_reverse_order
bool_with_responses, resp_skip, resp_limit, bool_recursive, bool_flagged_comments, bool_reverse_order,
merge_question_type_responses
).to_json
end

Expand Down
53 changes: 34 additions & 19 deletions presenters/thread.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ def to_hash(
resp_limit=nil,
recursive=true,
flagged_comments=false,
reverse_order=false
reverse_order=false,
merge_question_type_responses=false
)
raise ArgumentError unless resp_skip >= 0
raise ArgumentError unless resp_limit.nil? or resp_limit >= 1
Expand All @@ -48,7 +49,8 @@ def to_hash(
end
sorting_key_order = reverse_order ? -1 : 1
if with_responses
if @thread.thread_type.discussion? && resp_skip == 0 && resp_limit.nil?
if (@thread.thread_type.discussion? || (@thread.thread_type.question? && merge_question_type_responses)) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this check mean now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The objective of this pull request (PR) is to ensure that the API responses for discussion type threads and question type threads are consistent. To achieve this, the check is to verify if the merge_question_type_responses parameter is present in the API call and if the thread type is a question. If both conditions are met, the API response for question type threads should match that of discussion type threads.

resp_skip == 0 && resp_limit.nil?
if recursive
content = Comment.where(comment_thread_id: @thread._id).order_by({"sk" => sorting_key_order})
else
Expand All @@ -66,28 +68,41 @@ def to_hash(
end
case @thread.thread_type
when "question"
endorsed_responses = responses.where(endorsed: true)
non_endorsed_responses = responses.where(endorsed: false)
endorsed_response_info = get_paged_merged_responses(
if merge_question_type_responses
response_info = get_paged_merged_responses(
@thread._id,
endorsed_responses,
0,
nil,
recursive,
sorting_key_order
)
non_endorsed_response_info = get_paged_merged_responses(
@thread._id,
non_endorsed_responses,
responses,
resp_skip,
resp_limit,
recursive,
sorting_key_order
)
h["endorsed_responses"] = endorsed_response_info["responses"]
h["non_endorsed_responses"] = non_endorsed_response_info["responses"]
h["non_endorsed_resp_total"] = non_endorsed_response_info["response_count"]
h["resp_total"] = non_endorsed_response_info["response_count"] + endorsed_response_info["response_count"]
)
h["children"] = response_info["responses"]
h["resp_total"] = response_info["response_count"]
ayesha-waris marked this conversation as resolved.
Show resolved Hide resolved
else
endorsed_responses = responses.where(endorsed: true)
non_endorsed_responses = responses.where(endorsed: false)
endorsed_response_info = get_paged_merged_responses(
@thread._id,
endorsed_responses,
0,
nil,
recursive,
sorting_key_order
)
non_endorsed_response_info = get_paged_merged_responses(
@thread._id,
non_endorsed_responses,
resp_skip,
resp_limit,
recursive,
sorting_key_order
)
h["endorsed_responses"] = endorsed_response_info["responses"]
h["non_endorsed_responses"] = non_endorsed_response_info["responses"]
h["non_endorsed_resp_total"] = non_endorsed_response_info["response_count"]
h["resp_total"] = non_endorsed_response_info["response_count"] + endorsed_response_info["response_count"]
end
when "discussion"
response_info = get_paged_merged_responses(
@thread._id,
Expand Down
37 changes: 36 additions & 1 deletion spec/api/comment_thread_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ def thread_result(id, params)
it "limits responses when no skip/limit params given" do
@threads.each do |n, thread|
res = thread_result thread.id, {}
check_thread_response_paging_json thread, res, 0, nil, false
check_thread_response_paging_json thread, res, 0, nil, false, false
end
end

Expand Down Expand Up @@ -697,6 +697,41 @@ def thread_result(id, params)
end
end

context "response pagination" do
def thread_result(id, params)
get "/api/v1/threads/#{id}", params
expect(last_response).to be_ok
parse(last_response.body)
end

it "handles responses when merge_question_type_responses=true" do
User.all.delete
Content.all.delete
@user = create_test_user(999)
@threads = {}
@comments = {}
[201, 10, 3, 2, 1, 0].each do |n|
thread_key = "t#{n}"
thread = make_thread(@user, thread_key, DFLT_COURSE_ID, "pdq", :question)
@threads[n] = thread
n.times do |i|
# generate n responses in this thread
comment_key = "#{thread_key} r#{i}"
comment = make_comment(@user, thread, comment_key)
2.times do |j|
subcomment_key = "#{comment_key} c#{j}"
subcomment = make_comment(@user, comment, subcomment_key)
end
@comments[comment_key] = comment
end
end
@threads.each do |n, thread|
res = thread_result thread.id, {:merge_question_type_responses => true}
check_thread_response_paging_json thread, res, 0, nil, false, false, true
end
end
end

describe "PUT /api/v1/threads/:thread_id" do

before(:each) { init_without_subscriptions }
Expand Down
11 changes: 10 additions & 1 deletion spec/presenters/thread_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'spec_helper'

describe ThreadPresenter do

context "#to_hash" do
let(:default_resp_limit) { CommentService.config["thread_response_default_size"] }

Expand Down Expand Up @@ -167,6 +167,15 @@ def random_flag_abuses!(comment)
end
end

it "handles merge_question_type_responses=true" do
@threads_with_num_comments.each do |thread, num_comments|
is_endorsed = num_comments > 0 && endorse_responses
hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed, nil).to_hash(true, 0, default_resp_limit, true, false, true, true)
check_thread_result(@reader, thread, hash)
check_thread_response_paging(thread, hash, 0, default_resp_limit, false, false, true, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same check we have for before this new param was added!
Can we be more specific in the test to check merge_question_type_responses params impact on the results returned! Right now, we are sort of only checking if we get a response or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check_thread_response_paging is handling merge_question_type_responses impacts on the results returned

end
end

it "handles reversed_order and recursive with skip and limit" do
@threads_with_num_comments.each do |thread, num_comments|
is_endorsed = num_comments > 0 && endorse_responses
Expand Down
16 changes: 8 additions & 8 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,12 +254,12 @@ def check_unread_thread_result_json(thread, json_response)
check_thread_result(nil, thread, json_response, true)
end

def check_thread_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false, reverse_order=false)
case thread.thread_type
when "discussion"
check_discussion_response_paging(thread, hash, resp_skip, resp_limit, is_json, recursive, reverse_order)
when "question"
check_question_response_paging(thread, hash, resp_skip, resp_limit, is_json, recursive, reverse_order)
def check_thread_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false, reverse_order=false, merge_question_type_responses=false)

if (thread.thread_type == "discussion" || (thread.thread_type == "question" && merge_question_type_responses))
check_discussion_response_paging(thread, hash, resp_skip, resp_limit, is_json, recursive, reverse_order)
else
check_question_response_paging(thread, hash, resp_skip, resp_limit, is_json, recursive, reverse_order)
saadyousafarbi marked this conversation as resolved.
Show resolved Hide resolved
saadyousafarbi marked this conversation as resolved.
Show resolved Hide resolved
end
end

Expand Down Expand Up @@ -333,8 +333,8 @@ def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is
end
end

def check_thread_response_paging_json(thread, hash, resp_skip=0, resp_limit=nil, recursive=false, reverse_order=false)
check_thread_response_paging(thread, hash, resp_skip, resp_limit, true, recursive, reverse_order)
def check_thread_response_paging_json(thread, hash, resp_skip=0, resp_limit=nil, recursive=false, reverse_order=false, merge_question_type_responses=false)
check_thread_response_paging(thread, hash, resp_skip, resp_limit, true, recursive, reverse_order, merge_question_type_responses)
Copy link
Contributor

Choose a reason for hiding this comment

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

this new param passed is not taken as an input for the function check_thread_response_paging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This parameter is intended to be used exclusively in the case of question-type threads. If provided, it serves to check whether the generated response matches that of a discussion-type thread."

end

# general purpose factory helpers
Expand Down
Loading