Skip to content

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

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

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions models/attachment.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ func (a *Attachment) IncreaseDownloadCount() error {
return nil
}

// FileSize is returning the file datasize
func (a *Attachment) FileSize() (string, error) {
stats, err := os.Stat(AttachmentLocalPath(a.UUID))
if err != nil {
return "error", fmt.Errorf("AttachmentFileSize: %v", err)
}
result := float64(stats.Size()) / float64(1048576)
return fmt.Sprintf("%.1f", result) + " MB", nil
}

// AttachmentLocalPath returns where attachment is stored in local file
// system based on given UUID.
func AttachmentLocalPath(uuid string) string {
Expand Down Expand Up @@ -126,6 +136,11 @@ func GetAttachmentByUUID(uuid string) (*Attachment, error) {
return getAttachmentByUUID(x, uuid)
}

// GetAttachmentByReleaseIDFileName returns attachment by given releaseId and fileName.
func GetAttachmentByReleaseIDFileName(releaseID int64, fileName string) (*Attachment, error) {
return getAttachmentByReleaseIDFileName(x, releaseID, fileName)
}

func getAttachmentsByIssueID(e Engine, issueID int64) ([]*Attachment, error) {
attachments := make([]*Attachment, 0, 10)
return attachments, e.Where("issue_id = ? AND comment_id = 0", issueID).Find(&attachments)
Expand All @@ -142,6 +157,22 @@ func GetAttachmentsByCommentID(commentID int64) ([]*Attachment, error) {
return attachments, x.Where("comment_id=?", commentID).Find(&attachments)
}

/* Author : github.com/gdeverlant
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

getAttachmentByReleaseIDFileName return a file based on the the following infos:
- releaseID
- fileName
*/
func getAttachmentByReleaseIDFileName(e Engine, releaseID int64, fileName string) (*Attachment, error) {
attach := &Attachment{ReleaseID: releaseID, Name: fileName}
has, err := e.Get(attach)
if err != nil {
return nil, err
} else if !has {
return nil, ErrAttachmentNotExist{0, attach.UUID}
Copy link
Member

Choose a reason for hiding this comment

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

Change this to return nil, nil

Copy link
Contributor Author

@al-sabr al-sabr May 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i hope you are happy

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@al-sabr al-sabr May 11, 2017

Choose a reason for hiding this comment

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

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

}
return attach, nil
}

// DeleteAttachment deletes the given attachment and optionally the associated file.
func DeleteAttachment(a *Attachment, remove bool) error {
_, err := DeleteAttachments([]*Attachment{a}, remove)
Expand Down
35 changes: 35 additions & 0 deletions routers/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,41 @@ func Action(ctx *context.Context) {
ctx.Redirect(redirectTo)
}

// RedirectDownload return a file based on the the following infos:
Copy link
Member

@bkcsoft bkcsoft May 10, 2017

Choose a reason for hiding this comment

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

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

func RedirectDownload(ctx *context.Context) {
var (
vTag = ctx.Params("vTag")
fileName = ctx.Params("fileName")
)
if len(vTag) > 0 {
fmt.Println("Value of vTag %v", vTag)
}
if len(fileName) > 0 {
fmt.Println("Value of fileName %v", fileName)
}

var tagNames []string
tagNames = append(tagNames, vTag)
Copy link
Member

Choose a reason for hiding this comment

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

just do tagNames := []string{vTag}


curRepo := ctx.Repo.Repository

releases, err := models.GetReleasesByRepoIDAndNames(curRepo.ID, tagNames)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

No need, it'll fall through :)

ctx.Handle(500, "RedirectDownload -> Release not found", err)
return
}

if len(releases) == 1 {
release := releases[0]
att, err := models.GetAttachmentByReleaseIDFileName(release.ID, fileName)
if err != nil {
ctx.Handle(500, "RedirectDownload -> Attachment not found", err)
return
}
ctx.Redirect(setting.AppSubURL + "/attachments/" + att.UUID)
Copy link
Member

Choose a reason for hiding this comment

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

place this into if att != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}
Copy link
Member

Choose a reason for hiding this comment

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

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


// Download download an archive of a repository
func Download(ctx *context.Context) {
var (
Expand Down
4 changes: 4 additions & 0 deletions routers/routes/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

No need for this comment

m.Group("/releases", func() {
m.Get("/download/:vTag/:fileName", repo.RedirectDownload)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}, repo.MustBeNotBare, reqRepoWriter, context.RepoRef())
m.Group("/releases", func() {
m.Get("/new", repo.NewRelease)
m.Post("/new", bindIgnErr(auth.NewReleaseForm{}), repo.NewReleasePost)
Expand Down
22 changes: 13 additions & 9 deletions templates/repo/release/list.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,25 @@
<div class="download">
<h2>{{$.i18n.Tr "repo.release.downloads"}}</h2>
<ul class="list">
{{$tagname := .TagName}}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

this assignment is needed because tag range is not named

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

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

Choose a reason for hiding this comment

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

Remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lafriks lafriks May 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok it's removed now I will do that later

Copy link
Member

Choose a reason for hiding this comment

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

Remove old comments :)

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

Choose a reason for hiding this comment

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

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}})

Copy link
Contributor Author

@al-sabr al-sabr May 10, 2017

Choose a reason for hiding this comment

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

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 .

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@al-sabr al-sabr May 10, 2017

Choose a reason for hiding this comment

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

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.

</a>
</li>
{{end}}
{{end}}
<li>
<a href="{{$.RepoLink}}/archive/{{.TagName}}.zip" rel="nofollow"><i class="octicon octicon-file-zip"></i> {{$.i18n.Tr "repo.release.source_code"}} (ZIP)</a>
</li>
<li>
<a href="{{$.RepoLink}}/archive/{{.TagName}}.tar.gz"><i class="octicon octicon-file-zip"></i> {{$.i18n.Tr "repo.release.source_code"}} (TAR.GZ)</a>
</li>
{{if .Attachments}}
{{range .Attachments}}
<li>
<a target="_blank" rel="noopener" href="{{AppSubUrl}}/attachments/{{.UUID}}">
<span class="ui image octicon octicon-desktop-download" title='{{.Name}}'></span> {{.Name}}
</a>
</li>
{{end}}
{{end}}

</ul>
</div>
{{else}}
Expand Down