Show confirmation dialog if release created with existing tag #33316
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/L
github-actions
bot
added
modifies/translation
modifies/go
jackHay22
changed the title
wxiaoguang
reviewed
Jan 18, 2025
| @@ -38,6 +38,9 @@ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <div id="tag-warning" class="gt-dib gt-hidden" data-commit-url-stub="{{.RepoLink}}/commit"> | |||
| {{svg "octicon-alert-fill" 16}} Existing tag referencing <span class="gt-px-3">{{svg "octicon-git-commit" 16}} <a target="_blank" rel="noopener noreferrer" class="tag-warning-detail"></a></span> | |||
| </div> | |||
There was a problem hiding this comment.
I think this warning is good enough.
We can make it more prominent, then the modals could (should) be removed.
The problem is that:
- There are too many duplicate warnings when using the modal.
- There are too many problems in the modal related code (so remove the modals, then we do not need to spend time on it)
wxiaoguang
reviewed
Jan 18, 2025
| @@ -330,9 +330,9 @@ func newReleaseCommon(ctx *context.Context) { | |||
| ctx.Data["Title"] = ctx.Tr("repo.release.new_release") | |||
| ctx.Data["PageIsReleaseList"] = true | |||
|
|
|||
| tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) | |||
| tags, err := repo_model.GetTagMappingsByRepoID(ctx, ctx.Repo.Repository.ID) | |||
There was a problem hiding this comment.
I do not think it is right because GetTagNamesByRepoID does order by, but map is unordered,
wxiaoguang
reviewed
Jan 18, 2025
Comment on lines
+316
to
+319
| // CountReleasesByRepoID returns a number of releases matching FindReleaseOptions and RepoID. | ||
| func CountReleasesByRepoID(ctx context.Context, repoID int64, opts FindReleasesOptions) (int64, error) { | ||
| return db.GetEngine(ctx).Where(opts.ToConds()).Count(new(Release)) | ||
| } |
wxiaoguang
reviewed
Jan 18, 2025
| @@ -38,6 +38,9 @@ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <div id="tag-warning" class="gt-dib gt-hidden" data-commit-url-stub="{{.RepoLink}}/commit"> | |||
There was a problem hiding this comment.
There is no such gt-xxx helpers.
There was a problem hiding this comment.
Should be tw-inline-block tw-hidden
wxiaoguang
reviewed
Jan 18, 2025
| @@ -38,6 +38,9 @@ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <div id="tag-warning" class="gt-dib gt-hidden" data-commit-url-stub="{{.RepoLink}}/commit"> | |||
| {{svg "octicon-alert-fill" 16}} Existing tag referencing <span class="gt-px-3">{{svg "octicon-git-commit" 16}} <a target="_blank" rel="noopener noreferrer" class="tag-warning-detail"></a></span> | |||
silverwind
reviewed
Jan 22, 2025
| @@ -21,24 +23,66 @@ function initTagNameEditor() { | |||
| const el = document.querySelector('#tag-name-editor'); | |||
| if (!el) return; | |||
|
|
|||
| const tagWarning = document.querySelector('#tag-warning'); | |||
| const tagWarningDetailLinks = Array.from(document.querySelectorAll('.tag-warning-detail')); | |||
There was a problem hiding this comment.
Suggested change
| const tagWarningDetailLinks = Array.from(document.querySelectorAll('.tag-warning-detail')); | |
| const tagWarningDetailLinks = document.querySelectorAll<HTMLAnchorElement>('.tag-warning-detail'); |
This avoids the as further below and array convertion is unnecessary as for-of can iterate NodeList just fine.
silverwind
reviewed
Jan 22, 2025
Comment on lines
+75
to
+79
| for (const detail of tagWarningDetailLinks) { | ||
| const anchor = detail as HTMLAnchorElement; | ||
| anchor.href = `${tagURLStub}/${existingTags[value]}`; | ||
| anchor.textContent = existingTags[value].substring(0, 10); | ||
| } |
There was a problem hiding this comment.
Suggested change
| for (const detail of tagWarningDetailLinks) { | |
| const anchor = detail as HTMLAnchorElement; | |
| anchor.href = `${tagURLStub}/${existingTags[value]}`; | |
| anchor.textContent = existingTags[value].substring(0, 10); | |
| } | |
| for (const link of tagWarningDetailLinks) { | |
| link.href = `${tagURLStub}/${encodeURIComponent(existingTags[value])}`; | |
| link.textContent = existingTags[value].substring(0, 10); | |
| } |
Not totally sure whether encodeURIComponent is needed, but the other changes should be good.
There was a problem hiding this comment.
Most of the JS code could be removed, see #33316 (comment)
I do not think it's worth to spend more time on JS code for this case.
silverwind
reviewed
Jan 22, 2025
|
|
||
| const tagNameInput = document.querySelector<HTMLInputElement>('#tag-name'); | ||
| const hideTargetInput = function(tagNameInput: HTMLInputElement) { | ||
| const value = tagNameInput.value; | ||
| const tagHelper = document.querySelector('#tag-helper'); | ||
| if (existingTags.includes(value)) { | ||
| if (value in existingTags) { |
There was a problem hiding this comment.
Is existingTags an array? Then .includes was definitely fine. in should only be used to check key existance in objects.
|
@jackHay22 what do you think? Especially #33316 (review) , the new JS code is more complicated than it should be and quite fragile. I prefer to only use a prominent warning message, do not introduce more modal/JS code. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When creating a new release, a user may accidentally select an old tag. The current warning against this is fairly subtle. This PR adds a more descriptive warning and a confirmation dialog.
Screenshots