search

FriendsOfFlarum / merge-discussions

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

Fix failed merge due to post non sequential numbering #9

Closed imorland closed 4 years ago

imorland commented 4 years ago

Fixes #5

On the giffgaff community, we find that the previous fix does not reliably fix this issue. This was down to the number column on posts not incrementing sequentially (or posts being split/deleted).

By adding an additional sort on the discussion first, setting the number out of the current range, then continuing with the merge, everything works again 😄

dsevillamartin commented 4 years ago

Could we have that code run inside a DB transaction? That way if it fails later on the post numbers aren't completely wrong (I think that's how that works anyway, DB changes will be reverted if error is thrown).

imorland commented 4 years ago

Sure, just looking at a related issue at present (merge fails on very large threads)

I'll wrap this and the numbering fix in a transaction in the next commit

dsevillamartin commented 4 years ago

FYI, the transaction should apply for the whole code - not just each part separately, as the DB would still be modified if the second DB changes failed.

We can either wrap the whole thing in the transaction method or manually begin, commit & rollback them - this would use a try/catch, so it's basically the same thing with more code. See more @ https://laravel.com/docs/5.8/database#database-transactions.

imorland commented 4 years ago

I'm going to close this PR, as we're working on a different solution to fix this issue. I'll open a new PR once we have it resolved

dsevillamartin commented 4 years ago

@imorland Ok, thanks for trying nonetheless!

dsevillamartin commented 4 years ago

@imorland Was going through old issues, and I saw that the issue that this PR was gonna fix still exists. Do you have any updates on the solutin?

imorland commented 4 years ago

@datitisev not yet I'm afraid. The overall issue is reasonably high in our backlog, but I've not yet had another chance to look into it in any great detail yet