Prevent simultaneous editing of comments and issues

[metiftikci]

fixes #22907

Tested:

• issue content edit
• issue content change tasklist
• pull request content edit
• pull request change tasklist

[wxiaoguang]

Thank you for the feature! It is good for end users. However there are some blocking problems in my mind at the moment:

1. I do not think it's right to use XORM "version" here. Otherwise, all issue-model code needs to call NoVersionCheck again and again, and it might cause more breakings. (it is also related to point 3)
2. The comment content field is the same as issue content field, but comments are stored in another table.
   • Should we have a complete design before only "fixing" the problem for "issue"?
3. The "version" is quite unclear and would mislead developers.
   • I can see the "version" is only designed for "content"
   • But there are still other fields like "title", which can be changed simultaneously
   • And there are still other "related models" like "labels", "assignees" can be changed simultaneously
   • So I do not think we should use "version" for "issue content"

[metiftikci]

@wxiaoguang thank you for review. do you have any suggestion? maybe a 'content hash' colum as @drsybren said? or any generic solution?

[wxiaoguang]

TBH I haven't thought about the details carefully.

Some brief ideas (just my opinion):

1. Use ContentVersion, add it for both issue and comment tables, and this field is only used for "content editing".
2. Re-use the "issue content history", use the last issue content history record as reference.
3. Hash (sha256) the issue/comment's content as reference.

For the frontend mechanism: I would strongly suggest to make it independent of backend and totally transparent (do not calculate "version+1" in frontend). It could use an abstracted variable like data-content-version-tag, and frontend code only gets it from backend and sends it back to backend, then frontend doesn't need to share any knowledge with backend, and all of these "ideas" above work with this mechanism.

[lunny]

Thank you for the feature! It is good for end users. However there are some blocking problems in my mind at the moment:

1. I do not think it's right to use XORM "version" here. Otherwise, all issue-model code needs to call NoVersionCheck again and again, and it might cause more breakings. (it is also related to point 3)

2. The comment content field is the same as issue content field, but comments are stored in another table.
   
   * Should we have a complete design before only "fixing" the problem for "issue"?

3. The "version" is quite unclear and would mislead developers.
   
   * I can see the "version" is only designed for "content"
   * But there are still other fields like "title", which can be changed simultaneously
   * And there are still other "related models" like "labels", "assignees" can be changed simultaneously
   * So I do not think we should use "version" for "issue content"

I would agree since only content has the version, we should use a name like content_version column but not version. And looks like XORM based version will require careful changes in other places. Maybe just revert back to user-defined version. Sorry, @metiftikci .

But I don't think it's necessary to put comment content version changes in the same PR. Another PRs are better. Because most of the edit box needs column like this. i.e. comment, release note, user information, org information and etc.

[metiftikci]

@wxiaoguang changed Version to ContentVersion as int. Do i need to do something else? frontend gets new version number from backend

[wxiaoguang]

Overall it looks good to me. Thank you for the updates.

Actually, I still think we should make the "comment" also support "content version" in one PR: they share the same code (especially the frontend code), and I do not think it's worth to add another migration for "comment" table.

And one more thing, I think it's worth to write some test code to cover ChangeIssueContent's behavior.

[metiftikci]

@lunny i think fail is not related to pr, right ?

[lunny]

@lunny i think fail is not related to pr, right ?

I think so. Let me restart the failed jobs.

[lunny]

Looks like the CI failures are related.

[metiftikci]

My bad. fixed migration. now looks like fixed

[wxiaoguang]

(ps: no need to merge with every main commit manually, maintainers and the Gitea bot will do that 😄 )