-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Last Login for admin manage your users #121
Conversation
Good job, maybe you could try to rebase your commits into one commit ? |
@@ -119,6 +121,10 @@ func (u *User) BeforeUpdate() { | |||
u.UpdatedUnix = time.Now().Unix() | |||
} | |||
|
|||
func (u *User) AfterLogin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe UpdateLastLogin
will be a more appropriate name for this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xBAADF00D I chose this name because it is generic, it may be that in the future another implementation can be done in this same method. But I can change if is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@0xBAADF00D I can provide this, but Isn't it better to keep? Because first commit references the implementation on the route, while the last commit describes changes in the admin panel. |
Current coverage is 3.14% (diff: 0.00%)@@ master #121 diff @@
========================================
Files 33 33
Lines 7823 7827 +4
Methods 0 0
Messages 0 0
Branches 0 0
========================================
Hits 246 246
- Misses 7557 7561 +4
Partials 20 20
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add missing comments for linting
@@ -119,6 +121,10 @@ func (u *User) BeforeUpdate() { | |||
u.UpdatedUnix = time.Now().Unix() | |||
} | |||
|
|||
func (u *User) AfterLogin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide some comments for the linting of your function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
func (u *User) AfterLogin() { | ||
u.LastLoginUnix = time.Now().Unix() | ||
} | ||
|
||
func (u *User) AfterSet(colName string, _ xorm.Cell) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment for the function for the linting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I think you can keep your commits. Please make sure that your PRs are rebased once they're going to be merged, but that's it. You just need to squash if you feel like it's a good idea, but we don't force you to. |
@metalmatze redone all as one commit by reset |
@@ -119,6 +121,11 @@ func (u *User) BeforeUpdate() { | |||
u.UpdatedUnix = time.Now().Unix() | |||
} | |||
|
|||
// Set time to last login | |||
func (u *User) UpdateLastLogin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe name this SetLastLogin()
? Update
is a bit misleading, it would be apropriate if this actually saved the change to the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering, @0xBAADF00D asked to pu this name, originally was setAfterLogin()
Maybe UpdateLastLogin will be a more appropriate name for this function
I will provide fix for this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like SetLastLogin()
It's working fine. Can be merged after update. |
Yes. It's ready for merge. @0xBAADF00D @DblK Please check and |
Recommit with last changes |
LGTM |
1 similar comment
LGTM |
Could we dismiss or wait for @0xBAADF00D @DblK ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix gofmt lint-errors
@@ -119,6 +121,11 @@ func (u *User) BeforeUpdate() { | |||
u.UpdatedUnix = time.Now().Unix() | |||
} | |||
|
|||
// Set time to last login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has to start with the function-name, e.g // SetLastLogin sets the time of last login
. May sound stupid, but go fmt
requires it 😒
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkcsoft Okay, I will provide this
func (u *User) SetLastLogin() { | ||
u.LastLoginUnix = time.Now().Unix() | ||
} | ||
|
||
func (u *User) AfterSet(colName string, _ xorm.Cell) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it requires a comment that starts with // AfterSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkcsoft AfterSet
wasn't written by me, then I think that I don't know that this function make. I can take advantage of my commit and put it, but I'll need help to understand this function. Or I can put only // AfterSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkcsoft can be // AfterSet format data from database for display
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// AfterSet does magic
(ask @lunny since he knows XORM 🙂 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lunny now the ball is with you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @andreynering, comment these should be another PR.
@bkcsoft add comments on |
If that function doesn't change in this PR, you don't need to change that. Someone can send another PR for that. Em qua, 9 de nov de 2016 às 14:36, Joubert RedRat notifications@github.com
|
@DblK, hi, please review and it's ready for merge. |
^ |
* Add repo_blob This adds a new Repository.GetBlob(id) method for use by gitea. * This is a follow-up for PR go-gitea#121 to implement blob_api including full test coverage Signed-off-by: Berengar W. Lehr <Berengar.Lehr@kompetenztest.de>
This PR fixes #57
PS: Thanks @lunny for help on final with i18n.