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 all commits
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
29 changes: 28 additions & 1 deletion 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 @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

You should not have changed this

} else if !has {
return nil, ErrAttachmentNotExist{0, uuid}
}
Expand All @@ -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,18 @@ func GetAttachmentsByCommentID(commentID int64) ([]*Attachment, error) {
return attachments, x.Where("comment_id=?", commentID).Find(&attachments)
}

// getAttachmentByReleaseIDFileName return a file based on the the following infos:
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
31 changes: 31 additions & 0 deletions routers/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,37 @@ func Action(ctx *context.Context) {
ctx.Redirect(redirectTo)
}

// RedirectDownload return a file based on the following infos:
func RedirectDownload(ctx *context.Context) {
var (
vTag = ctx.Params("vTag")
fileName = ctx.Params("fileName")
)
tagNames := []string{vTag}
curRepo := ctx.Repo.Repository
releases, err := models.GetReleasesByRepoIDAndNames(curRepo.ID, tagNames)
if err != nil {
if models.IsErrAttachmentNotExist(err) {
ctx.Error(404)
return
}
ctx.Handle(500, "RedirectDownload: Failed to get attachment", err)
return
}
if len(releases) == 1 {
release := releases[0]
att, err := models.GetAttachmentByReleaseIDFileName(release.ID, fileName)
if err != nil {
ctx.Handle(404, "RedirectDownload -> Attachment not found", err)
Copy link
Member

Choose a reason for hiding this comment

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

This is supposed to be a 500 error

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
}

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.

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

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 give up !!!! I will stop pushing my features on this repo I will use my features myself.
Thanx for the inputs.

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.

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

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

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

// Download download an archive of a repository
func Download(ctx *context.Context) {
var (
Expand Down
1 change: 1 addition & 0 deletions routers/routes/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,7 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Post("/delete", repo.DeleteMilestone)
}, reqRepoWriter, context.RepoRef())
m.Group("/releases", func() {
m.Get("/download/:vTag/:fileName", repo.RedirectDownload)
m.Get("/new", repo.NewRelease)
m.Post("/new", bindIgnErr(auth.NewReleaseForm{}), repo.NewReleasePost)
m.Post("/delete", repo.DeleteRelease)
Expand Down
21 changes: 12 additions & 9 deletions templates/repo/release/list.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,24 @@
<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>
<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