Skip to content

Fix avatar enumable #1049

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 25, 2017
Merged

Fix avatar enumable #1049

merged 2 commits into from
Feb 25, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 25, 2017

fix #718

@lunny lunny added the type/bug label Feb 25, 2017
@lunny lunny added this to the 1.1.0 milestone Feb 25, 2017
strk
strk reviewed Feb 25, 2017
View reviewed changes
models/migrations/migrations.go
@@ -88,6 +88,8 @@ var migrations = []Migration{
NewMigration("add external login user", addExternalLoginUser),
// v19 -> v20
NewMigration("generate and migrate Git hooks", generateAndMigrateGitHooks),
// v20 -> v21
NewMigration("use new avtar path name for security reason", useNewNameAvatars),
Copy link
Member

Choose a reason for hiding this comment

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

Typo

@strk
Copy link
Member

strk commented Feb 25, 2017

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 25, 2017
@andreynering
Copy link
Contributor

LGTM, but I tried in a dev environment with a user that didn't have an avatar, and it generated a new one. Is this the desired behavior?

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 25, 2017
@lunny
Copy link
Member Author

lunny commented Feb 25, 2017

No. I think that's not a desired behavior. The migration will rename all the avatars on the data/avatars from to and the upload avatar name will be not . Only these two changes.

@appleboy
Copy link
Member

LGTM

@lunny lunny merged commit 80f900e into go-gitea:master Feb 25, 2017
@gsantner
Copy link

gsantner commented Feb 26, 2017 •
edited
Loading

Pulled to my installation. Now user avatars are not loaded, and wrong org ones.
Somehow for organizations the same icons get always loaded (there should be different ones).
Did a restart, and gitea has write access to avatar dir.

grafik

@lunny
Copy link
Member Author

lunny commented Feb 26, 2017

Please fire an issue.

@lunny lunny deleted the lunny/fix_avatar_enumable branch February 26, 2017 13:24
lunny added a commit to lunny/gitea that referenced this pull request Feb 26, 2017
@lunny
bug fixed for org avatar caused by go-gitea#1049
5652844
lunny added a commit that referenced this pull request Feb 27, 2017
@lunny
Bug fixed for org avatar caused by #1049 (#1070)
522f194 
* bug fixed for org avatar caused by #1049

* use isfile only
@richmahn
Copy link
Contributor

richmahn commented May 7, 2017 •
edited
Loading

This PR is making all my custom avatars (I have 2890 users, many which have custom avatars uploaded) be the same avatar! I added a few Printfs to the Migration, migrating up from v15, and I see this:

USER TO UPDATE: 4613, {f295aae8bd4b6dca44ad1c35bcf99ad7 true}
2017/05/07 11:07:51 [I] [SQL] UPDATE `user` SET `avatar` = ?  [f295aae8bd4b6dca44ad1c35bcf99ad7]

As you can see, there is no WHERE condition to set id = 4613!

@lunny
Copy link
Member Author

lunny commented May 7, 2017

@richmahn what's your database?

@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@strk strk strk left review comments

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

Uploaded custom avatars enumerable and downloadable w/o login despite "Require Sign In View" being set
7 participants
@lunny @strk @andreynering @appleboy @gsantner @richmahn @tboerger