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

Add the tag list page to the release page #12096

Merged
merged 40 commits into from
Nov 2, 2020

Conversation

a1012112796
Copy link
Member

@a1012112796 a1012112796 commented Jun 30, 2020

  • Add the tags list view
  • Add the delete tag way on ui
  • Not delete tag and clear message when delete a release
  • Not show single tag in release list
    example view:

tmp1

tmp2

@a1012112796 a1012112796 mentioned this pull request Jun 30, 2020
7 tasks
@UnlimitedCookies
Copy link

Looks awesome! How about putting a trashcan symbol instead of text in brackets behind the Release version?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 30, 2020
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 30, 2020
@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label Jun 30, 2020
@zeripath zeripath added this to the 1.13.0 milestone Jun 30, 2020
@mrsdizzie
Copy link
Member

This doesn't delete a tag from the git repo it just removes reference from the releases table. This means the tag still exists, you can still view it in tags list on main repo view, and still create a release from it. If releases for repo get re-synced it will show up on the releases page again too. Tags and releases are different you can't use the same code for removing a release -- release only exists in the DB but tag exists within git itself. So this doesn't fixed the linked issue and will probably mislead people into thinking a tag has been deleted when it hasn't. So I think this would have to actually delete the tag in git itself to count.

If it was changed to do actually delete the tag, then I would say:

Agree it should use a trash can instead of text -- Maybe another icon after TAR.GZ like this:

Screen Shot 2020-07-01 at 9 52 51 AM

I know it was probably to reuse the existing translation, but I think the warning should not use the phrase 'deleting a release' since we make a difference between release and tag and this is about deleting a single tag.

@a1012112796

This comment has been minimized.

@a1012112796

This comment has been minimized.

@mrsdizzie
Copy link
Member

mrsdizzie commented Jul 1, 2020

ah I see I missed that 'true' but the error I see is like this:

DeleteReleaseByID: LoadAttributes: user does not exist [uid: 0, name: , keyid: 0]

When a repo is migrated it makes 'releases' from existing tags but sets the PublisherID to 0. I believe this is true for all tags in the release table. Then when you try to delete one of those tags it shows the error above.

rel.Repo = repo
if err = rel.LoadAttributes(); err != nil {
return fmt.Errorf("LoadAttributes: %v", err)

gitea/models/release.go

Lines 45 to 60 in 858c35b

func (r *Release) loadAttributes(e Engine) error {
var err error
if r.Repo == nil {
r.Repo, err = GetRepositoryByID(r.RepoID)
if err != nil {
return err
}
}
if r.Publisher == nil {
r.Publisher, err = getUserByID(e, r.PublisherID)
if err != nil {
return err
}
}
return getReleaseAttachments(e, r)
}

Current code is only for releases that are created and assume all release have publisher information, but tags on release page do not.

@a1012112796

This comment has been minimized.

@mrsdizzie
Copy link
Member

Maybe DeleteReleaseByID can check if release.IsTag and not loadAttributes then?

@a1012112796
Copy link
Member Author

Maybe DeleteReleaseByID can check if release.IsTag and not loadAttributes then?

I found an question is that the pushUpdateAddTags not us user who push tags as release's Publisher but use the author of the commit which is be taged. I wonder whether it's right?

commit, err := tag.Commit()
if err != nil {
return fmt.Errorf("Commit: %v", err)
}
sig := tag.Tagger
if sig == nil {
sig = commit.Author
}
if sig == nil {
sig = commit.Committer
}
var author *models.User
var createdAt = time.Unix(1, 0)
if sig != nil {
var ok bool
author, ok = emailToUser[sig.Email]
if !ok {
author, err = models.GetUserByEmailContext(ctx, sig.Email)
if err != nil && !models.IsErrUserNotExist(err) {
return fmt.Errorf("GetUserByEmail: %v", err)
}
if author != nil {
emailToUser[sig.Email] = author
}
}
createdAt = sig.When
}
commitsCount, err := commit.CommitsCount()
if err != nil {
return fmt.Errorf("CommitsCount: %v", err)
}
rel, has := relMap[lowerTag]
if !has {
rel = &models.Release{
RepoID: repo.ID,
Title: "",
TagName: tags[i],
LowerTagName: lowerTag,
Target: "",
Sha1: commit.ID.String(),
NumCommits: commitsCount,
Note: "",
IsDraft: false,
IsPrerelease: false,
IsTag: true,
CreatedUnix: timeutil.TimeStamp(createdAt.Unix()),
}
if author != nil {
rel.PublisherID = author.ID
}

@UnlimitedCookies
Copy link

On the UI end:

Agree it should use a trash can instead of text -- Maybe another icon after TAR.GZ like this:
Screen Shot 2020-07-01 at 9 52 51 AM

I don't like to show it on here, which is more like delete attachments..

I agree the trashcan symbol needs to visually stand out from the release attachments.
Why not place it behind the version number (where the text previously was)?
And to make it stand out more maybe color it (red?) and change the size.

@lunny
Copy link
Member

lunny commented Jul 2, 2020

So if somebody wants only to remove release but not tag, how should he do?

@a1012112796
Copy link
Member Author

So if somebody wants only to remove release but not tag, how should he do?

Yes, the current logic isn't reasonable, and we should provide the option to choose whether to delete the tag when you delete the release, but that's not the aim of this PR.

@lunny
Copy link
Member

lunny commented Jul 2, 2020

How about a new tab named tags on the home page? And user could manage tags there.

@a1012112796
Copy link
Member Author

How about a new tab named tags on the home page? And user could manage tags there.

Maybe can use same style with github:
image

@lunny
Copy link
Member

lunny commented Jul 2, 2020

I would like to replace 25MiB as tags.
图片

* Add the tags list view
* Add the delete tag way on ui
* Not delete tag and clear message when delete a release

Signed-off-by: a1012112796 <1012112796@qq.com>
@a1012112796 a1012112796 changed the title Add delete button for single tag on releases list page Add the tag list page to the release page Jul 3, 2020
@a1012112796 a1012112796 requested a review from zeripath July 3, 2020 15:46
@a1012112796
Copy link
Member Author

This PR has been changed. Please review it again. Thanks.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 4, 2020

I would like to replace 25MiB as tags.
图片

That information is already available a little higher, on Releases tab. On the other hand, the size is not visible anywhere else.

@UnlimitedCookies
Copy link

UnlimitedCookies commented Jul 4, 2020

I would like to replace 25MiB as tags.
图片

That information is already available a little higher, on Releases tab. On the other hand, the size is not visible anywhere else.

Why not add the number of tabs after the number of commits and when you click on tags it will show a list of tags similar to the commit list?

This would mean that the shown row would look like this: 1477 Commits --- 45 Tags --- 25 Branches --- 25 MiB

Or you add a selector for tag/release in the releases section - like Github does it.

web_src/less/_base.less Outdated Show resolved Hide resolved
web_src/less/_base.less Outdated Show resolved Hide resolved
Co-authored-by: silverwind <me@silverwind.io>
@silverwind
Copy link
Member

silverwind commented Nov 1, 2020

Good to go? (after rebase)

@lunny please re-review

@lafriks
Copy link
Member

lafriks commented Nov 1, 2020

tests are failing:

release_test.go:59: 
        	Error Trace:	release_test.go:59
        	            				release_test.go:104
        	Error:      	Not equal: 
        	            	expected: 3
        	            	actual  : 2
        	Test:       	TestCreateReleaseDraft

@a1012112796
Copy link
Member Author

tests are failing:

release_test.go:59: 
        	Error Trace:	release_test.go:59
        	            				release_test.go:104
        	Error:      	Not equal: 
        	            	expected: 3
        	            	actual  : 2
        	Test:       	TestCreateReleaseDraft

It's not a problem, because the new item added by #13358 is a tag, which will not be shown in the release list, so the actual number is right :)

id: 3
repo_id: 1
publisher_id: 2
tag_name: "delete-tag"
lower_tag_name: "delete-tag"
target: "master"
title: "delete-tag"
sha1: "65f1bf27bc3bf70f64657658635e66094edbcb4d"
num_commits: 10
is_draft: false
is_prerelease: false
is_tag: true
created_unix: 946684800

@a1012112796
Copy link
Member Author

The ui is broken after merge master. @silverwind Would you please have a look , Thanks.
image

@silverwind
Copy link
Member

Please pull in silverwind@74a04ac which fixes those and does a few more enhancements:

Screen Shot 2020-11-01 at 17 54 10

Screen Shot 2020-11-01 at 17 53 55

Screen Shot 2020-11-01 at 17 58 01

Screen Shot 2020-11-01 at 17 58 13

@silverwind
Copy link
Member

Also pull in silverwind@71aa21e for some more tweaks on the title:

Screen Shot 2020-11-01 at 18 04 21

@codecov-io
Copy link

Codecov Report

Merging #12096 into master will decrease coverage by 0.02%.
The diff coverage is 23.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12096      +/-   ##
==========================================
- Coverage   42.13%   42.11%   -0.03%     
==========================================
  Files         691      691              
  Lines       75968    76002      +34     
==========================================
- Hits        32010    32006       -4     
- Misses      38710    38744      +34     
- Partials     5248     5252       +4     
Impacted Files Coverage Δ
models/release.go 54.26% <0.00%> (+0.23%) ⬆️
services/release/release.go 44.44% <ø> (+1.72%) ⬆️
routers/repo/release.go 24.71% <16.21%> (-2.01%) ⬇️
routers/routes/routes.go 84.36% <100.00%> (+0.07%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
services/pull/temp_repo.go 26.59% <0.00%> (-3.20%) ⬇️
services/pull/patch.go 53.97% <0.00%> (-1.71%) ⬇️
modules/log/file.go 73.60% <0.00%> (-1.61%) ⬇️
modules/queue/workerpool.go 58.77% <0.00%> (-1.23%) ⬇️
services/pull/check.go 49.63% <0.00%> (+0.72%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06268dc...4cd0000. Read the comment docs.

@lafriks
Copy link
Member

lafriks commented Nov 2, 2020

@lunny need your review

web_src/less/_base.less Outdated Show resolved Hide resolved
@lunny lunny dismissed their stale review November 2, 2020 14:09

Will not block this.

@lafriks
Copy link
Member

lafriks commented Nov 2, 2020

🚀

@techknowlogick techknowlogick merged commit b687707 into go-gitea:master Nov 2, 2020
@a1012112796 a1012112796 deleted the delet_tag branch November 3, 2020 01:12
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
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. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.