FriendsOfFlarum / merge-discussions

Merge two or more discussions into one
MIT License
10 stars 7 forks source link

Duplicate post numbering when merging discussions causes error #5

Closed dsevillamartin closed 2 years ago

dsevillamartin commented 5 years ago
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '292-3' for key 'posts_discussion_id_number_unique'

It occurs when re-numbering the posts in the new discussion.

Not sure why it only occurs sometimes. Perhaps because of the order in which posts are processed (by ID / creation date).

rob006 commented 5 years ago

This bug is pretty consistent for me - it happens always when merge will require posts renumbering (you need to merge posts older than last post in target discussion).

rob006 commented 5 years ago

I opened PR with fix: https://github.com/FriendsOfFlarum/merge-discussions/pull/7

I'm not using Laravel, so please make sure that I didn't do anything stupid. :P

matteocontrini commented 5 years ago

It doesn't appear to be fixed: https://discuss.flarum.org/d/19460-friendsofflarum-merge-discussions/31

@rob006 any idea of what could help in debugging?

rob006 commented 5 years ago

@matteocontrini I edited some Laravel files to log (simple file_put_contents()) all queries before executing it, and I figure out what is happening. I have no idea how to setup proper debugging with Flarum, and Eloquent is just pure magic...

imorland commented 4 years ago

Been spending a little time today looking into this.

So it appears this issue occurs when there are gaps in the post number sequence. Not sure of the cause yet, but potentially could be:

My first idea is to run a check for the number of the last post in the discussion, and if this differs from the overall count, then re-number the posts before attempting the merge.

@datitisev I know you worked on this extension, do you have any insights on these findings?

I'll continue to investigate and play around with some ideas. I'm determined to get a fix for this ASAP 😄

imorland commented 4 years ago

Got this working, at least for our use case. #9

imorland commented 2 years ago

Potentially addressed by https://github.com/FriendsOfFlarum/merge-discussions/pull/30 - tagged as 1.0.1