Skip to content

Fix issue #1730 - Now all actions are modified #1744

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
Closed
wants to merge 1 commit into from
Closed

Fix issue #1730 - Now all actions are modified #1744

wants to merge 1 commit into from

Conversation

@DblK
Copy link
Member

@DblK DblK commented May 17, 2017

Action in Dashboard are renamed when the user is renamed. This fix #1730.
lunny
lunny requested changes May 18, 2017
View reviewed changes
models/action.go
Cols("act_user_name", "repo_user_name").
Where("act_user_id = ?", id).
Update(ac)
log.Info("%v", err)
Copy link
Member

@lunny lunny May 18, 2017

Choose a reason for hiding this comment

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

err maybe nil
Copy link
Member Author

@DblK DblK May 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I will remove this line
models/action.go
ActUserName: newUserName,
}
_, err := x.
Cols("act_user_name", "repo_user_name").
Copy link
Member

@lunny lunny May 18, 2017

Choose a reason for hiding this comment

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

Should only update act_user_name. And add another SQL update action set repo_user_name=? where repo_user_name=?
Copy link
Member Author

@DblK DblK May 18, 2017

Choose a reason for hiding this comment

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

It won't be possible because I don't have the oldUserName, you want me to add it to the function?
I am not sure to understand how you want me to code it.
@lunny
Copy link
Member

lunny commented May 18, 2017

And a migration should be added
@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 18, 2017
@lunny lunny added the type/bug label May 18, 2017
@lunny lunny added this to the 1.2.0 milestone May 18, 2017
@DblK
Copy link
Member Author

DblK commented May 18, 2017

What migration should I do?
Update act_user_name according to the username of the act_user_id?

I have a hard time coding the migration but I can try to do it.
@lunny
Copy link
Member

lunny commented May 22, 2017

Maybe you should add a migration like the below.

x.Exec("update action set act_user_name = (select name from user where user.id = action.act_user_id)")
@lunny lunny mentioned this pull request May 23, 2017
Merged
@DblK
Copy link
Member Author

DblK commented May 23, 2017

Since #1779 try to achieve the same thing in other way, I will close this PR in favor of this other.
@lunny what do you think?
@lunny
Copy link
Member

lunny commented May 23, 2017

@DblK I agree with you.
@DblK DblK closed this May 23, 2017
@lunny lunny removed this from the 1.2.0 milestone May 24, 2017
@lunny lunny added issue/duplicate The issue has already been reported. and removed type/bug labels May 24, 2017
@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.

Labels

issue/duplicate The issue has already been reported. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.

3 participants

@DblK @lunny @tboerger