Added URL mapping for Release attachments like on github.com #1700

al-sabr · 2017-05-10T02:04:56Z

Implement a Git Hub style URL mapping for the attachment files in Release.

See screenshot :

lunny · 2017-05-10T02:11:32Z

An integration test is welcome.

lafriks · 2017-05-10T05:38:52Z

templates/repo/release/list.tmpl

+									{{if .Attachments}}
+										{{range $attachment := .Attachments}}
+										<li>
+											<!-- {{AppSubUrl}}/attachments/{{$attachment.UUID}} -->


Remove commented out code

I will not remove this out because I want to add for each release an option to display like github url or leave it the old way.

than it has to be done before it gets merged in master

Ok it's removed now I will do that later

lafriks · 2017-05-10T05:45:14Z

routers/repo/repo.go

+	curRepo := ctx.Repo.Repository
+
+	releases, err := models.GetReleasesByRepoIDAndNames(curRepo.ID, tagNames)
+	if err != nil {


I think there should be also added or statement to check len(releases) == 0

No need, it'll fall through :)

lafriks · 2017-05-10T05:47:10Z

models/attachment.go

@@ -142,6 +157,22 @@ func GetAttachmentsByCommentID(commentID int64) ([]*Attachment, error) {
 	return attachments, x.Where("comment_id=?", commentID).Find(&attachments)
 }

+/*	Author : github.com/gdeverlant


No need to add Author in comments, it will be visible in git history anyway

lafriks · 2017-05-10T05:49:52Z

Also need to fix this error: repo.go:271:1: comment on exported function RedirectDownload should be of the form "RedirectDownload ..."

Otherwise good job 👍

bkcsoft · 2017-05-10T07:47:22Z

routers/repo/repo.go

@@ -268,6 +268,34 @@ func Action(ctx *context.Context) {
 	ctx.Redirect(redirectTo)
 }

+//	RedirectDownload return a file based on the the following infos:


"// RedirectDownload returns a file based on tag and filename" :)

bkcsoft · 2017-05-10T07:47:54Z

routers/repo/repo.go

+		fileName = ctx.Params("fileName")
+	)
+	var tagNames []string
+	tagNames = append(tagNames, vTag)


just do tagNames := []string{vTag}

bkcsoft · 2017-05-10T07:49:14Z

templates/repo/release/list.tmpl

@@ -53,21 +53,25 @@
 							<div class="download">
 								<h2>{{$.i18n.Tr "repo.release.downloads"}}</h2>
 								<ul class="list">
+									{{$tagname := .TagName}}


Since you're using a named range, you don't need this assignment

this assignment is needed because tag range is not named

bkcsoft

Just a few things before LGTM :)

bkcsoft · 2017-05-10T09:46:26Z

routers/repo/repo.go

+		}
+		ctx.Redirect(setting.AppSubURL + "/attachments/" + att.UUID)
+	}
+}


End the function with ctx.Handle(404, "RedirectDownload", nil) otherwise you get a 500 error if the release doesn't exist :)

bkcsoft · 2017-05-10T09:46:43Z

routers/routes/routes.go

@@ -486,6 +486,10 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Get("/:id/:action", repo.ChangeMilestonStatus)
 			m.Post("/delete", repo.DeleteMilestone)
 		}, reqRepoWriter, context.RepoRef())
+		// New redirection from fake url


No need for this comment

bkcsoft · 2017-05-10T09:47:16Z

routers/routes/routes.go

@@ -486,6 +486,10 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Get("/:id/:action", repo.ChangeMilestonStatus)
 			m.Post("/delete", repo.DeleteMilestone)
 		}, reqRepoWriter, context.RepoRef())
+		// New redirection from fake url
+		m.Group("/releases", func() {
+			m.Get("/download/:vTag/:fileName", repo.RedirectDownload)


Move this to the Group just below (since they are identical)

bkcsoft · 2017-05-10T09:47:46Z

templates/repo/release/list.tmpl

+									{{if .Attachments}}
+										{{range $attachment := .Attachments}}
+										<li>
+											<!-- {{AppSubUrl}}/attachments/{{$attachment.UUID}} -->


Remove old comments :)

bkcsoft · 2017-05-10T10:57:20Z

routers/repo/repo.go

+	curRepo := ctx.Repo.Repository
+	releases, err := models.GetReleasesByRepoIDAndNames(curRepo.ID, tagNames)
+	if err != nil {
+		ctx.Handle(404, "RedirectDownload -> Release not found", err)


This is supposed to be a 500 error

This is done

bkcsoft · 2017-05-10T10:57:25Z

routers/repo/repo.go

+		release := releases[0]
+		att, err := models.GetAttachmentByReleaseIDFileName(release.ID, fileName)
+		if err != nil {
+			ctx.Handle(404, "RedirectDownload -> Attachment not found", err)


This is supposed to be a 500 error

It will return ErrAttachmentNotExist error when no attachment by that filename exists. I think logic need to remade for GetAttachmentByReleaseIDFileName to return nil, nil when no attachment is found, so that it could be easily returned 404 on nil, nil and 500 on nil, err

But it can also be a DB-error, in which case it should be an error...

if err != nil { if _, ok := err.(*ErrAttachmentNotFound); ok { ctx.Handle(404, "RedirectDownload: Attachment not found", err) return } ctx.Handle(500, "RedirectDownload: Failed to get attachment", err) return }

Guys I'm getting pissed off just past the full code and I copy and past it... 🥇

I give up !!!! I will stop pushing my features on this repo I will use my features myself.
Thanx for the inputs.

bkcsoft test your code before you post it because it is broken.
You didn't declared ErrAttachmentNotFound
I'm fedup with your recommendations

lafriks · 2017-05-10T11:09:56Z

routers/repo/repo.go

+		release := releases[0]
+		att, err := models.GetAttachmentByReleaseIDFileName(release.ID, fileName)
+		if err != nil {
+			ctx.Handle(404, "RedirectDownload -> Attachment not found", err)


It will return ErrAttachmentNotExist error when no attachment by that filename exists. I think logic need to remade for GetAttachmentByReleaseIDFileName to return nil, nil when no attachment is found, so that it could be easily returned 404 on nil, nil and 500 on nil, err

lafriks · 2017-05-10T11:23:21Z

models/attachment.go

+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrAttachmentNotExist{0, attach.UUID}


Change this to return nil, nil

why ?????????????????????

So you can later understand and check if file was not found or if there was some internal error that you have to return error 500

Instead of telling me how to change it why don't you post the full code so that I get what you try to mean.

i hope you are happy

you changed it in wrong function and wrong line. You need to change:
return nil, ErrAttachmentNotExist{0, attach.UUID} to return nin, nin

No you expressed it wrong that's why I could not understand!

lafriks · 2017-05-10T11:29:13Z

routers/repo/repo.go

+			ctx.Handle(404, "RedirectDownload -> Attachment not found", err)
+			return
+		}
+		ctx.Redirect(setting.AppSubURL + "/attachments/" + att.UUID)


place this into if att != nil {

lafriks · 2017-05-10T12:35:37Z

templates/repo/release/list.tmpl

+										<li>
+											<a target="_blank" rel="noopener" href="{{$.RepoLink}}/releases/download/{{$tagname}}/{{$attachment.Name}}">
+												<span class="ui image octicon octicon-desktop-download" title='{{$attachment.Name}}'></span> {{$attachment.Name}} 
+												<a target="_blank" href="{{$.RepoLink}}/releases/download/{{$tagname}}/{{$attachment.Name}}" class="ui right"><span class="ui image octicon octicon-file-binary" title='{{$attachment.Name}}'></span>{{$attachment.FileSize}}</a>


There should not be tag inside tag. File size shoud be moved out of link, seperated with space and placed in brackets: </a> ({{$attachment.FileSize}})

Well it is not forbidden in HTML and there is a reason so I leave it. The font displayed is different from the front of the row with the filename so that is the reason I put this inside of another .

but why would you need that? Why first link is not enough?

Because the font is displayed differently from the other file description. The MB is some kind of Time New Roman. It is convenient for the user to give him a choice of either clicking on the file size, the icon or the filename to download.

It is UI experience.

ethantkoenig · 2017-05-10T13:38:09Z

routers/repo/repo.go

@@ -268,6 +268,33 @@ func Action(ctx *context.Context) {
 	ctx.Redirect(redirectTo)
 }

+// RedirectDownload return a file based on the the following infos:


repeated the

al-sabr

have a look

al-sabr · 2017-05-10T21:53:43Z

models/attachment.go

+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrAttachmentNotExist{0, attach.UUID}


Instead of telling me how to change it why don't you post the full code so that I get what you try to mean.

al-sabr · 2017-05-10T21:55:00Z

routers/repo/repo.go

+			ctx.Handle(404, "RedirectDownload -> Attachment not found", err)
+			return
+		}
+		ctx.Redirect(setting.AppSubURL + "/attachments/" + att.UUID)


al-sabr · 2017-05-10T21:55:48Z

routers/routes/routes.go

@@ -486,6 +486,10 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Get("/:id/:action", repo.ChangeMilestonStatus)
 			m.Post("/delete", repo.DeleteMilestone)
 		}, reqRepoWriter, context.RepoRef())
+		// New redirection from fake url
+		m.Group("/releases", func() {
+			m.Get("/download/:vTag/:fileName", repo.RedirectDownload)


lafriks · 2017-05-11T04:50:50Z

models/attachment.go

@@ -104,7 +114,7 @@ func getAttachmentByUUID(e Engine, uuid string) (*Attachment, error) {
 	attach := &Attachment{UUID: uuid}
 	has, err := e.Get(attach)
 	if err != nil {
-		return nil, err
+		return nil, nil


You should not have changed this

al-sabr · 2017-05-11T11:33:21Z

can you guys remove this pull request totally I will not work on this anymore

pgaskin · 2017-05-11T11:48:02Z

It cannot be deleted, but it can be closed.

al-sabr · 2017-05-11T11:58:30Z

See for new pull request #1707

Added URL mapping for Release attachments like on github.com

2e6c39b

lunny added the type/enhancement An improvement of existing functionality label May 10, 2017

lunny added this to the 1.3.0 milestone May 10, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 10, 2017

Removed formatting directive in Println call

1a539a6

lafriks requested changes May 10, 2017

View reviewed changes

bkcsoft suggested changes May 10, 2017

View reviewed changes

Made changes from Staff code review

aefa570

bkcsoft suggested changes May 10, 2017

View reviewed changes

ergü added 3 commits May 10, 2017 11:56

Reverted changes from Staff code review mistake

0a9a812

Added 404 errors instead of 500

5ca8651

Removed author infos from code

84d22d8

bkcsoft suggested changes May 10, 2017

View reviewed changes

lafriks requested changes May 10, 2017

View reviewed changes

cez81 mentioned this pull request May 10, 2017

Added URL mapping for Release attachments like on github.com #1699

Closed

lafriks reviewed May 10, 2017

View reviewed changes

Function end with 404 error and cleanup.

52b4dea

lafriks reviewed May 10, 2017

View reviewed changes

ergü added 2 commits May 10, 2017 14:17

Minor correction

98cb0d2

Wishlist added

7aa17f7

lafriks requested changes May 10, 2017

View reviewed changes

ethantkoenig suggested changes May 10, 2017

View reviewed changes

al-sabr commented May 10, 2017

View reviewed changes

Loads of wishlist for code review

4119816

lafriks reviewed May 11, 2017

View reviewed changes

al-sabr closed this May 11, 2017

al-sabr deleted the git-style-release-url-attachments branch May 11, 2017 23:00

al-sabr mentioned this pull request May 12, 2017

Added URL mapping for Release attachments like on github.com #1707

Merged

lunny removed this from the 1.3.0 milestone May 12, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Added URL mapping for Release attachments like on github.com #1700

Added URL mapping for Release attachments like on github.com #1700

Uh oh!

Conversation

al-sabr commented May 10, 2017 • edited by bkcsoft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lafriks May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lafriks commented May 10, 2017

Uh oh!

bkcsoft May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkcsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

al-sabr May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

al-sabr May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

al-sabr commented May 10, 2017 •

edited by bkcsoft

Loading

lafriks May 10, 2017 •

edited

Loading

bkcsoft May 10, 2017 •

edited

Loading

al-sabr May 10, 2017 •

edited

Loading

al-sabr May 10, 2017 •

edited

Loading

al-sabr May 10, 2017 •

edited

Loading

al-sabr May 11, 2017 •

edited

Loading

al-sabr May 10, 2017 •

edited

Loading