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

Repository avatars #6986

Merged

Conversation

@sergey-dryabzhinsky
Copy link
Contributor

commented May 19, 2019

  • first variant of code from old work for gogs
  • add migration 87
  • add new option in app.ini
  • add en-US locale string
  • add new class in repository.less

For issue #694 and #5625

Repository avatars
- first variant of code from old work for gogs
- add migration 87
- add new option in app.ini
- add en-US locale string
- add new class in repository.less

@sergey-dryabzhinsky sergey-dryabzhinsky referenced this pull request May 19, 2019

Closed

Repository logo #694

4 of 6 tasks complete
@sergey-dryabzhinsky

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Should I commit also index.css?

@lunny

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@sergey-dryabzhinsky yes, please.

@lunny

This comment has been minimized.

Copy link
Member

commented May 19, 2019

add new option in app.ini.sample and docs

@lunny lunny added the kind/feature label May 19, 2019

sergey-dryabzhinsky added some commits May 19, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 19, 2019

Codecov Report

Merging #6986 into master will decrease coverage by <.01%.
The diff coverage is 37.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6986      +/-   ##
==========================================
- Coverage   41.43%   41.42%   -0.01%     
==========================================
  Files         442      443       +1     
  Lines       59701    59865     +164     
==========================================
+ Hits        24735    24797      +62     
- Misses      31729    31819      +90     
- Partials     3237     3249      +12
Impacted Files Coverage Δ
models/migrations/migrations.go 1.52% <ø> (ø) ⬆️
routers/repo/setting.go 9.34% <0%> (-0.73%) ⬇️
routers/user/setting/profile.go 40% <0%> (-0.82%) ⬇️
models/migrations/v87.go 0% <0%> (ø)
modules/setting/setting.go 48.15% <100%> (+0.55%) ⬆️
routers/routes/routes.go 82.91% <100%> (+0.23%) ⬆️
models/repo.go 47.21% <47.31%> (ø) ⬆️
modules/log/colors_router.go 83.33% <0%> (-4.17%) ⬇️
models/repo_list.go 73.09% <0%> (+1.01%) ⬆️

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 d749404...141d542. Read the comment docs.

sergey-dryabzhinsky added some commits May 19, 2019

Fixes:
- remove old avatar file on upload
- use ID in name of avatar file - users may upload same files
- add simple tests
@sergey-dryabzhinsky

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Ready for review

Show resolved Hide resolved models/migrations/v87.go Outdated
}

if err != nil {
return fmt.Errorf("Error changing mirror interval column type: %v", err)

This comment has been minimized.

Copy link
@zeripath

zeripath May 19, 2019

Contributor

If we do need this migration you need to change this line.

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 19, 2019

Author Contributor

Fixed

Show resolved Hide resolved routers/repo/setting.go Outdated
@@ -727,3 +730,53 @@ func init() {
panic(err)
}
}

// UpdateAvatarSetting update repo's avatar
// FIXME: limit size.

This comment has been minimized.

Copy link
@zeripath

zeripath May 19, 2019

Contributor

You must limit size before we merge - just stick a config in it doesn't need a great UI.

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 19, 2019

Author Contributor

Fixed

sergey-dryabzhinsky added some commits May 19, 2019

Update avatar upload checks:
- add file size check
- add new option
- update config docs
- add new string to en-us locale
Fixes:
- use FileHEader field for check file size
- add new test - upload big image
@techknowlogick

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I've just dismissed my review so I'm not blocking this PR anymore.

@@ -166,6 +172,9 @@ type Repository struct {
CloseIssuesViaCommitInAnyBranch bool `xorm:"NOT NULL DEFAULT false"`
Topics []string `xorm:"TEXT JSON"`

// Avatar
Avatar string `xorm:"VARCHAR(2048)"`

This comment has been minimized.

Copy link
@techknowlogick

techknowlogick May 28, 2019

Member

Does the full URL need to be stored in DB? For user avatars this makes sense due to gravatar (and other external avatar services), but for repo avatars it follows the following patter setting.AppSubURL + "/repo-avatars/" + repo.Avatar

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 28, 2019

Author Contributor

You mean that it easily fits into 250 symbols?
ID+MD5 ~ (10-20) + 32.
64 symbols must be enough I think.

This comment has been minimized.

Copy link
@techknowlogick

techknowlogick May 28, 2019

Member

Yes, rather than storing the full URL, just the filename could be stored.

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 28, 2019

Author Contributor

Done in 4ae9383

// Users can upload the same image to other repo - prefix it with ID
// Then repo will be removed - only it avatar file will be removed
repo.Avatar = fmt.Sprintf("%d-%x", repo.ID, md5.Sum(data))
if _, err := x.ID(repo.ID).Cols("avatar").Update(repo); err != nil {

This comment has been minimized.

Copy link
@lunny

lunny May 29, 2019

Member

sess. not x.

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 29, 2019

Author Contributor

Done in c007a9a

@sergey-dryabzhinsky

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Also rewrite models.repo.DeleteAvatar - now it uses Session too.

@lafriks
Copy link
Member

left a comment

Two code readability issues but otherwise LG-TM

func (repo *Repository) DeleteAvatar() error {

// Avatar exists
if len(repo.Avatar) > 0 {

This comment has been minimized.

Copy link
@lafriks

lafriks May 29, 2019

Member

Please return as soon as possible for code readability:

Suggested change
if len(repo.Avatar) > 0 {
if len(repo.Avatar) == 0 {
return nil
}

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 29, 2019

Author Contributor

Point taken.
Done in f53758a

// UpdateAvatarSetting update repo's avatar
func UpdateAvatarSetting(ctx *context.Context, form auth.AvatarForm) error {
ctxRepo := ctx.Repo.Repository
if form.Avatar != nil {

This comment has been minimized.

Copy link
@lafriks

lafriks May 29, 2019

Member

Same here (do not use else):

Suggested change
if form.Avatar != nil {
if form.Avatar == nil {
// No avatar is uploaded but setting has been changed to enable
// No random avatar here.
if !com.IsFile(ctxRepo.CustomAvatarPath()) {
log.Trace("No avatar was uploaded for repo: %d. Default icon will appear instead.", ctxRepo.ID)
}
return nil
}

This comment has been minimized.

Copy link
@sergey-dryabzhinsky

sergey-dryabzhinsky May 29, 2019

Author Contributor

Done in f53758a

@lunny

lunny approved these changes May 29, 2019

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels May 29, 2019

@lunny lunny added this to the 1.9.0 milestone May 29, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@sergey-dryabzhinsky Thanks for your good job!!!

@richmahn

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Yes, this looks great! Was going to review but looks like you have reviewers, and a few minor details to fix. This is definitely something my organization will find useful!

@lunny lunny added the changelog label May 29, 2019

@sergey-dryabzhinsky

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@richmahn same here.
I've tired to make war with bloated GitLab in my company.
So I decided to take an action.

Love to use gitea at home to manage my pet projects.
Now it will be even better.

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels May 29, 2019

@techknowlogick techknowlogick merged commit 3fd1883 into go-gitea:master May 30, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@saitho

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

When no repository avatar was assigned it will render an empty img tag in repository list.
<img class="ui avatar image" src="">

EDIT: Fixed in #7087

@sergey-dryabzhinsky

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

ohh... missed that then merge code changes from gogs.

@maznu

This comment has been minimized.

Copy link

commented Jul 6, 2019

Really hyped for 1.9! Thanks, everyone! 👍

@saitho

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Really hyped for 1.9!

Definitely! To me 1.9 seems to be much larger than previous feature releases. 👍

jeffliu27 added a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019

Repository avatars (go-gitea#6986)
* Repository avatars

- first variant of code from old work for gogs
- add migration 87
- add new option in app.ini
- add en-US locale string
- add new class in repository.less

* Add changed index.css, remove unused template name

* Update en-us doc about configuration options

* Add comments to new functions, add new option to docker app.ini

* Add comment for lint

* Remove variable, not needed

* Fix formatting

* Update swagger api template

* Check if avatar exists

* Fix avatar link/path checks

* Typo

* TEXT column can't have a default value

* Fixes:

- remove old avatar file on upload
- use ID in name of avatar file - users may upload same files
- add simple tests

* Fix fmt check

* Generate PNG instead of "static" GIF

* More informative comment

* Fix error message

* Update avatar upload checks:

- add file size check
- add new option
- update config docs
- add new string to en-us locale

* Fixes:

- use FileHEader field for check file size
- add new test - upload big image

* Fix formatting

* Update comments

* Update log message

* Removed wrong style - not needed

* Use Sync2 to migrate

* Update repos list view

- bigger avatar
- fix html blocks alignment

* A little adjust avatar size

* Use small icons for explore/repo list

* Use new cool avatar preparation func by @lafriks

* Missing changes for new function

* Remove unused import, move imports

* Missed new option definition in app.ini

Add file size check in user/profile avatar upload

* Use smaller field length for Avatar

* Use session to update repo DB data, update DeleteAvatar - use session too

* Fix err variable definition

* As suggested @lafriks - return as soon as possible, code readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.