From 9db339cbd49118f2c31d8599763a28110fc2620c Mon Sep 17 00:00:00 2001 From: ayeshoali Date: Tue, 6 Feb 2024 18:23:13 +0500 Subject: [PATCH 1/3] feat: updated api to allow ordering on endorsed status --- api/comment_threads.rb | 5 +++- presenters/thread.rb | 53 +++++++++++++++++++++++++++--------------- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/api/comment_threads.rb b/api/comment_threads.rb index ac9feaeddf..690bb7fefb 100644 --- a/api/comment_threads.rb +++ b/api/comment_threads.rb @@ -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"]) @@ -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 diff --git a/presenters/thread.rb b/presenters/thread.rb index 538e4cf5ef..37f8f7c9f9 100644 --- a/presenters/thread.rb +++ b/presenters/thread.rb @@ -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 @@ -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)) && + resp_skip == 0 && resp_limit.nil? if recursive content = Comment.where(comment_thread_id: @thread._id).order_by({"sk" => sorting_key_order}) else @@ -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"] + 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, From 00a8a04287c20ea111f3a0befdd2ac7d1aee0940 Mon Sep 17 00:00:00 2001 From: ayeshoali Date: Thu, 29 Feb 2024 21:40:13 +0500 Subject: [PATCH 2/3] test: fixed test cases --- spec/api/comment_thread_spec.rb | 9 ++++++++- spec/presenters/thread_spec.rb | 11 ++++++++++- spec/spec_helper.rb | 16 ++++++++-------- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/spec/api/comment_thread_spec.rb b/spec/api/comment_thread_spec.rb index 17137be5a5..d6a2c9f6b5 100644 --- a/spec/api/comment_thread_spec.rb +++ b/spec/api/comment_thread_spec.rb @@ -669,7 +669,14 @@ 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 + + it "handles responses when merge_question_type_responses param is given" do + @threads.each do |n, thread| + res = thread_result thread.id, {} + check_thread_response_paging_json thread, res, 0, nil, false, true end end diff --git a/spec/presenters/thread_spec.rb b/spec/presenters/thread_spec.rb index 1e7d5962fe..10bc199541 100644 --- a/spec/presenters/thread_spec.rb +++ b/spec/presenters/thread_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe ThreadPresenter do - + context "#to_hash" do let(:default_resp_limit) { CommentService.config["thread_response_default_size"] } @@ -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) + 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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fbf43dd582..12042a10b8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -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) end end @@ -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) end # general purpose factory helpers From 9a1d9cc19a40abf0f908ed53ff0c407c29a43e62 Mon Sep 17 00:00:00 2001 From: ayeshoali Date: Fri, 1 Mar 2024 15:05:13 +0500 Subject: [PATCH 3/3] test: fixed failing test case --- spec/api/comment_thread_spec.rb | 42 +++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/spec/api/comment_thread_spec.rb b/spec/api/comment_thread_spec.rb index d6a2c9f6b5..daf99af631 100644 --- a/spec/api/comment_thread_spec.rb +++ b/spec/api/comment_thread_spec.rb @@ -673,13 +673,6 @@ def thread_result(id, params) end end - it "handles responses when merge_question_type_responses param is given" do - @threads.each do |n, thread| - res = thread_result thread.id, {} - check_thread_response_paging_json thread, res, 0, nil, false, true - end - end - it "skips the specified number of responses" do @threads.each do |n, thread| res = thread_result thread.id, {:resp_skip => 1} @@ -704,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 }