Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent simultaneous editing of comments and issues #31053

Merged
merged 21 commits into from
May 27, 2024

Conversation

metiftikci
Copy link
Contributor

@metiftikci metiftikci commented May 22, 2024

fixes #22907

Tested:

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

issue-content-edit

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 22, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 22, 2024
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations modifies/js labels May 22, 2024
models/issues/issue.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2024
@lunny lunny added this to the 1.23.0 milestone May 23, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 24, 2024

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
Copy link
Contributor Author

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

@wxiaoguang
Copy link
Contributor

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
Copy link
Member

lunny commented May 24, 2024

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
Copy link
Contributor Author

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

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 25, 2024

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.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 26, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 26, 2024
@metiftikci metiftikci changed the title Add issue version to prevent change issue content simultaneously Prevent simultaneous editing of comments and issues May 26, 2024
@metiftikci
Copy link
Contributor Author

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

@lunny
Copy link
Member

lunny commented May 26, 2024

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

I think so. Let me restart the failed jobs.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@lunny lunny enabled auto-merge (squash) May 27, 2024 07:55
@lunny
Copy link
Member

lunny commented May 27, 2024

Looks like the CI failures are related.

@lunny lunny disabled auto-merge May 27, 2024 12:56
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@metiftikci
Copy link
Contributor Author

My bad. fixed migration. now looks like fixed

@wxiaoguang
Copy link
Contributor

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

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@lunny lunny enabled auto-merge (squash) May 27, 2024 15:04
@lunny lunny merged commit aa92b13 into go-gitea:main May 27, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 30, 2024
* giteaofficial/main: (30 commits)
  Azure blob storage support (go-gitea#30995)
  Use repo as of renderctx's member rather than a repoPath on metas (go-gitea#29222)
  Ignore FindRecentlyPushedNewBranches err (go-gitea#31164)
  [skip ci] Updated translations via Crowdin
  Fix markup preview (go-gitea#31158)
  Swap word order in Comment and Close (go-gitea#31148)
  Fix push multiple branches error with tests (go-gitea#31151)
  Use vertical layout for multiple code expander buttons (go-gitea#31122)
  Remove duplicate `ProxyPreserveHost` in Apache httpd doc (go-gitea#31143)
  [skip ci] Updated translations via Crowdin
  Add an immutable tarball link to archive download headers for Nix (go-gitea#31139)
  Improve mobile review ui (go-gitea#31091)
  Add topics for repository API (go-gitea#31127)
  Add missed return after `ctx.ServerError` (go-gitea#31130)
  Fix API repository object format missed (go-gitea#31118)
  Fix DashboardRepoList margin (go-gitea#31121)
  Update JS dependencies (go-gitea#31120)
  [skip ci] Updated translations via Crowdin
  Prevent simultaneous editing of comments and issues (go-gitea#31053)
  Update demo site location from try.gitea.io -> demo.gitea.com (go-gitea#31054)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/js modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simultaneous edits to an issue/PR lead to data loss
4 participants