Marking closed topic solved with solve_topics_auto_close_hours set causes topic to be reopened


(Joshua Rosenfeld) #1

Just set solved topics auto close hours on one of my sites. Ran into an issue when a moderator marked a closed topic solved. The topic was re-opened. This was quite unexpected, and caused a year old topic to resurface to the top of the /latest. If a topic is already closed, marking a post as the solution should not cause the topic to re-open.


(Joshua Rosenfeld) #2

I’m usually not this quick to ask “any progress” on a report - but this has become a real issue on my site. We really like the auto-close timer - but reopening closed topics is really bad!!! I’m going to try and dig into the code for this, but I’d really appreciate some help. To me this is #bug.


(Joshua Rosenfeld) #3

OK…so I think this is the line I need to modify:

I don’t know ruby, so it’s going to take some googling but I think I just want to do something like this:

if (auto_close_hours = SiteSetting.solved_topics_auto_close_hours) > 0 and topic !closed

(David Taylor) #4

That looks almost right to me. I think the syntax should be:

and !topic.closed

(Joshua Rosenfeld) #5

Would that be valid ruby if just tacked onto the end? Or do I need to surround the first clause of the if statement with parenthesis? Not sure what the order of operations is in Ruby.


(David Taylor) #6

Think what you’ve got should be fine - ! and > have higher precedence than and

https://ruby-doc.org/core-2.2.0/doc/syntax/precedence_rdoc.html


(Joshua Rosenfeld) #7

Wonderful, thanks @David_Taylor!

PR submitted:


(Joshua Rosenfeld) #8

So… @zogstrip said a test is needed before he can merge it. Besides not knowing Ruby - I’ve never had to write a test before. I’m going to start looking at other plugins, but help would be appreciated.

Aside: it really is awful how “theoretical” most CS degrees are. I can talk all about algorithms, but when it comes to practical stuff like version control, or tests - wish me luck!


(Régis Hanol) #9

Look at how that file is tested and use another test as starting point :wink:


(Joshua Rosenfeld) #10

Yep - I see the existing test, and I understand the basic structure…currently trying to find a test that deals with an open/closed topic.


(Joshua Rosenfeld) #11

OK. So does this work?

I’m thinking something like:

require 'rails_helper'

RSpec.describe "Managing Posts solved status" do
  let(:topic) { Fabricate(:topic) }
  let(:topic2) { Fabricate(:topic) } # Make a second topic
  let(:user) { Fabricate(:trust_level_4) }
  let(:p1) { Fabricate(:post, topic: topic) }
  let(:p2) { Fabricate(:post, topic: topic2) } # Add a post to the second topic 

# previous test:
  before do
    SiteSetting.allow_solved_on_all_topics = true
  end

  describe 'accepting a post as the answer' do
    before do
      sign_in(user)
      SiteSetting.solved_topics_auto_close_hours = 2
    end

    it 'can mark a post as the accepted answer correctly' do
      xhr :post, "/solution/accept", id: p1.id

      expect(p1.reload.custom_fields["is_accepted_answer"]).to eq("true")

      expect(topic.public_topic_timer.status_type).to eq(TopicTimer.types[:close])

      expect(topic.public_topic_timer.execute_at)
        .to be_within(1.second).of(Time.zone.now + 2.hours)

      expect(topic.public_topic_timer.based_on_last_post).to eq(true)
    end
# end of previous test
    it 'does not set a timer when the topic is closed' do
      topic2.update!(closed: true)
      xhr :post, "/solution/accept", id: p2.id 

      expect(p2.reload.custom_fields["is_accepted_answer"]).to eq("true")

      expect(topic2.reload.public_topic_timer).to eq(nil)
      
      expect(topic2.reload.closed).to eq(true)
    end
  end
end

(Joshua Rosenfeld) #12

I’ve added this spec to the PR. Hopefully this is in fact what @zogstrip was looking for…

Edit: and this is merged! Thanks @zogstrip for your help. Régis did bring up a concern about archived topics, but I’m not sure how topic timers affect archived topics. Trying to test on my dev instance, but I’m running into issues getting it updated.