PostRevisor가 삭제된 주제의 게시물을 수정할 수 없음

기여 버그
1

tl;dr

For some posts, calling PostRevisor sets the post_id to nil. Am I crazy?

Answer: No. PostRevisor accesses post.topic, which is nil for a post in a deleted topic. And then it sets post.topic to nil, which in turn sets post.topic_id to nil.

I think PostRevisor should get the topic like this:

  @topic = topic || Topic.with_deleted.find_by(id: post_topic_id)

rather than like this:

  @topic = topic

post=Post.find_by(topic_id: 179227, post_number: 12)
post.topic_id => 179227
pr=PostRevisor.new(post)
post.topic_id => nil

The Full Story

I’m working on a script that fixes goo.gl links (the service is sunsetting soon, so the script finds goo.gl links, fetches what they are redirected to and gsubs the goo.gl URL with the one it’s directed to. It mostly works.

But

For a bunch of posts, everything looks like it’s going just fine, but then PostRevisor fails because post.acting_user is nil. And then, in my rescue, it seems that the topic_id is nil, but it’s not the post that’s nil, because it still has a post_number.

      begin
        puts "Revising (#{count}/#{total_posts}) https://mysite.com/t/#{post.topic_id}/#{post.post_number}"
        puts "missing topic_id for post #{post.id}" if !post.topic_id
        next if !post.topic_id
        PostRevisor.new(post).revise!(system_user, raw: new_raw, **revision_options)
      rescue => e
        puts "cannot revise (number: #{count} https://tw.forumosa.com/t/#{post.topic_id}/#{post.post_number}): #{e}"
      end

FIXING!!: https://goo.gl/maps/XaNG
B7qaZGzhBmM78 -----> https://www.google.com/maps/place/%E6%AD%A5%E9%81%93%E5%92%96%E5%95%A1%E9%A4%A8Cafe+Strada/@22.6300414,120.3153591,17z/d
ata=!3m1!4b1!4m5!3m4!1s0x346e04944a9b3471:0x520c1f01c3d62e57!8m2!3d22.6301115!4d120.3175543?shorturl=1
Revising (680/1773) https://mysite.com/t/207069/1817
cannot revise (number: 680 https://mysite.com/t//1817): undefined method `acting_user=' for nil

For the vast majority of posts, this works just fine, but for some subset of them, it fails like this. And if I run the code by hand line-by-line I get the same thing. It’s starting to look like, though, that if I do a pr=PostRevisor.new(post) I see that the post in the pr record has no topic_id and then if I inspect the post it now has topic_id set to nil.

1개의 좋아요
2

Out of interest, any reason why you aren’t just editing the Post model directly?

3

Because if you are changing 3000 posts with gsub and a complicated regex with a bunch of edge cases (fix: goo.gl, http://goo.gl, https://goo.gl, but don’t touch https://maps.app.goo.gl, or https://map.goo.gl, and maybe you get rate limited by goo.gl, and on and on) you might want to get back the post before you totally screwed it up!

It’s been very cool to be able to look at the edits and see the before and after and be able to revert! One version would make https://maps.app.goo.gl/abd12 into something like https://maps.app.https://maps.google.com/;lkajw3rpoazse;flknmase;faijserfasefklasdfa, for example.

1개의 좋아요
4

that makes sense :slight_smile:

1개의 좋아요
5

Some years ago there was some similar script that I tried to get CDCK to run for some client and they said (in my words, not theirs) “Dude. You think we’re going to run your code that just willy-nilly changes a zillion posts and there’s no way to undo it? Think again.”

1개의 좋아요
6

Yeah, other approach is to run it on a backup in a controlled area. But I love your solution.

1개의 좋아요
7

So here’s the initializer:

So somehow the post is getting there and though post_id has a value, post does not?

8

This I cannot reproduce.

The last post.topic_id is not nil and as expected a repetition of the prior result

9

Yeah. It worked on most of the posts. There’s something funky with some of them.

[63] pry(main)> post.topic_id
=> 179227
[64] pry(main)> post.topic
=> nil
[65] pry(main)>

I’m pretty sure that’s not supposed to happen. :person_shrugging:

But wait:

 Topic.find(post.topic_id)
ActiveRecord::RecordNotFound: Couldn't find Topic with 'id'=179227 [WHERE "topics"."deleted_at" IS NULL]
from /var/www/discourse/vendor/bundle/ruby/3.3.0/gems/activerecord-7.2.2.1/lib/active_record/relation/finder_methods.rb:428:in `raise_record_not_found_exception!'

The topic is deleted.

So then the question is whether it’s a bug that you can’t revise posts in deleted topics.

I guess I’m not going to care and have my script check post.topic for nil rather than post.topic_id

1개의 좋아요
10

Yeah some nasty lack of integrity there with the foreign key!

I think some of the guard-rails(ahem) are switched off because the tables are usually too large.

Maybe someone performed a topic.delete not destroy. Oops.

11

게시글을 검색할 때 Post.where("raw like '%goo.gl%'")를 사용했는데, 이는 삭제된 토픽에 있는 게시글까지 반환합니다. 그런 게시글들은 topic_id는 있지만 topic은 없습니다. Topic.find가 삭제된 토픽도 반환할 수 있는 방법이 있다고 생각합니다 (관리자는 삭제된 토픽을 볼 수 있기 때문에, 이 부분이 정말 혼란스러웠습니다. 그 작은 휴지통 아이콘은 놓치기 쉽거든요.)

그래서 이렇습니다:

deleted_topic = Topic.with_deleted.find_by(id: 123)

아마 PostRevisor를 호출하기 전에 이렇게 해서 게시글을 업데이트했어야 했을지도 모릅니다.

하지만 UX상에서는 삭제된 토픽의 게시글을 수정할 수 있으므로, 다음 코드

def initialize(post, topic = post.topic)
  @post = post
  @topic = topic
  # Topic 인스턴스가 하나만 있도록 보장
  post.topic = topic
end

은 다음과 같이 되어야 한다고 생각합니다.

def initialize(post, topic = post.topic)
  @post = post
  @topic = topic || Topic.with_deleted.find_by(id: post_topic_id)
  # Topic 인스턴스가 하나만 있도록 보장
  post.topic = topic
end

혹시 다른 사람도 이것이 버그라고 생각할 수 있으니 #contribute:bug로 옮기겠습니다.

하지만 이건 동작합니다:

        if !post.topic # 삭제된 토픽의 게시글은 topic이 없어 PostRevisor를 깨뜨림
           post.topic = Topic.with_deleted.find_by(id: post.topic_id)
           next if !post.topic
        end
        PostRevisor.new(post).revise!(system_user, raw: new_raw, **revision_options)

Rails를 크래시 시킨 게시글이 3개의 토픽에 있었습니다. 그건 포기합니다. :slight_smile:

1개의 좋아요
12

And another version of the above that does a deleted_topic = Topic.with_deleted.find_by(id: 123) to update post.topic also works.

Still seems like a bug, though. Or maybe since core does something else to manage this, it’s not a bug.

1개의 좋아요
13

Or a missing Feature (and therefore spec)?

Online, this will never be called for Posts in deleted Topics?

I agree, though, that it should manage that properly :thinking:

14

Perhaps

They’re clearly doing something to allow it to work on posts in deleted topics, probably like I am.

Yeah. Someone who makes such decisions can move this back to Development if they think that’s appropriate.

15

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.