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

Use form for admin purge user #21070

Merged
merged 5 commits into from
Sep 12, 2022
Merged

Use form for admin purge user #21070

merged 5 commits into from
Sep 12, 2022

Conversation

jolheiser
Copy link
Member

Fixes #20998

The basic modal actions were set up for basic confirmation-style modals, however this modal also has a special form input, which instead requires a form in the modal itself.
The basic modal actions are indirectly controlled by JS and are simple <div> elements, whereas this requires a <button> to submit.

This appears to be similar to how we do it in (for example) the repo deletion modal.

@jolheiser jolheiser added this to the 1.18.0 milestone Sep 5, 2022
@lunny lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Sep 5, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 5, 2022
@@ -151,7 +151,7 @@

<div class="field">
<button class="ui green button">{{.locale.Tr "admin.users.update_profile"}}</button>
<div class="ui red button show-modal" data-modal="#delete-user-modal" data-url="{{$.Link}}/delete" data-id="{{.User.ID}}">{{.locale.Tr "admin.users.delete_account"}}</div>
<div class="ui red button show-modal" data-modal="#delete-user-modal">{{.locale.Tr "admin.users.delete_account"}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things in my mind:

  1. Here the data-id="{{.User.ID}}" is removed, is it correct? IIRC the JS code will fill some data-xxx attributes into the form fields.
  2. In Add option to purge users #18064, the button's CSS class name was changed from delete-button to show-modal, I am not sure what are the differences and concerns. Now the dialog layout is changed again, could @zeripath take a look at them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will re-test, but you may be right about 1.
About 2, delete-button is how we link it to the generic JS version afaik.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show-modal is also a generic JS version .... indeed, the code for show-modal and delete-button are quite similar, that's why I am confused about they two.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've ran some tests locally again.

The reason data-id is no longer needed is because this page has the user ID in the URL, which is what is used when deleting.

At the very least, I was able to:

  1. Attempt to delete a user and correctly get back "You can't, they still have repos", and then
  2. Select the purge option, which deleted them successfully.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, it seems to happen to work 😀

If @zeripath doesn't have more questions, I will vote L-G-T-M later.

@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 Sep 10, 2022
@zeripath
Copy link
Contributor

make lgtm work

@zeripath zeripath merged commit 2854031 into go-gitea:main Sep 12, 2022
@jolheiser jolheiser deleted the admin-delete branch September 12, 2022 20:52
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 13, 2022
* upstream/main:
  Update docs comparison.zh-cn.md (go-gitea#21035)
  Use form for admin purge user (go-gitea#21070)
  Make labels clickable in the comments section. (go-gitea#21137)
  Remove fomantic image module (go-gitea#21145)
  [skip ci] Updated translations via Crowdin
  Show .editorconfig errors in frontend (go-gitea#21088)
  Update JS dependencies and lint (go-gitea#21144)
  Fix PlantUML example in document (go-gitea#21142)
  chore(security): Support Go Vulnerability Management (go-gitea#21139)
  [skip ci] Updated licenses and gitignores
  [skip ci] Updated translations via Crowdin
  Improve commit status icons (go-gitea#21124)
  Center-aligning content of WebAuthN page (go-gitea#21127)
  Allow poster to choose reviewers (go-gitea#21084)
  Generate go-licenses during tidy again (go-gitea#21108)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin can not delete users, the form is not submitted when clicking "yes"
6 participants