Skip to content

Drop redundant columns from issue_user table #638

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 1 commit into from
Feb 3, 2017
Merged

Drop redundant columns from issue_user table #638

merged 1 commit into from
Feb 3, 2017

Conversation

ethantkoenig
Copy link
Member

@ethantkoenig ethantkoenig commented Jan 10, 2017 •
edited
Loading

Drops repo_id, milestone_id, is_poster, is_closed columns. See #636

@ethantkoenig ethantkoenig force-pushed the refactor/issue_user branch 2 times, most recently from 7f9b9f7 to 4da8292 Compare January 10, 2017 22:57
@appleboy
Copy link
Member

Build error.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 11, 2017
@ethantkoenig
Copy link
Member Author

@appleboy Fixed, thanks for the heads up!

@lunny lunny added this to the 1.1.0 milestone Jan 11, 2017
lunny
lunny reviewed Jan 11, 2017
View reviewed changes
models/issue.go
@@ -166,6 +166,18 @@ func (issue *Issue) LoadAttributes() error {
return issue.loadAttributes(x)
}

// GetIsRead load the `IsRead` field of the issue
func (issue *Issue) GetIsRead(userID int64) error {
Copy link
Member

Choose a reason for hiding this comment

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

Since we have notification system, it seems is_read is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The IsRead field is still used (in /templates/repo/issue/list.tmpl).

While it might make sense to eventually remove the is_read column, it seems outside the scope of this PR.

lunny
lunny reviewed Jan 11, 2017
View reviewed changes
models/issue.go
IsRead bool
IsAssigned bool
IsMentioned bool
Copy link
Member

@lunny lunny Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If IsMentioned and IsAssigned have been notified to user, the whole table will not be needed. @andreynering

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need to support searching for issues that a particular user is mentioned in, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think IsAssigned is not actually redundant?

Copy link
Member Author

@ethantkoenig ethantkoenig Jan 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The issue table has an assignee_id column, and gitea only supports one assignee per repo issue

Copy link
Member

Choose a reason for hiding this comment

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

Oh. We need keep IsAssigned currently and drop assignee_id on issue in future.

@ethantkoenig
Copy link
Member Author

I've un-removed the is_assigned column.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 14, 2017
andreynering
andreynering reviewed Jan 14, 2017
View reviewed changes
models/migrations/v16.go
if _, err := x.Exec("ALTER TABLE issue_user DROP COLUMN is_poster"); err != nil {
return err
}
if _, err := x.Exec("ALTER TABLE issue_user DROP COLUMN is_closed"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

On Gogs, the convention was never delete columns in the DB to prevent break anything, specially if user tries to rollback to a previous version of Gogs/Gitea.

Should we keep this convention?

Copy link
Member

Choose a reason for hiding this comment

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

^
Let's mark it depreciated firstly and don't use them.

@andreynering
Copy link
Contributor

One more comment. Otherwise LGTM.

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 14, 2017
@ethantkoenig
Copy link
Member Author

@lunny @andreynering I've removed the migration; is there anything I have to do to explicitly mark the columns as deprecated?

@andreynering
Copy link
Contributor

@ethantkoenig

Is there anything I have to do to explicitly mark the columns as deprecated?

No, just removing the struct field is OK.

@andreynering
Copy link
Contributor

Build error

@appleboy
Copy link
Member

appleboy commented Feb 3, 2017

LGTM

@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 3, 2017
@lunny lunny merged commit 68bdaf0 into go-gitea:master Feb 3, 2017
@lunny lunny mentioned this pull request Feb 5, 2017
Closed
@ethantkoenig ethantkoenig deleted the refactor/issue_user branch February 9, 2017 18:12
@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

@lunny lunny lunny left review comments

@andreynering andreynering andreynering 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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ethantkoenig @appleboy @andreynering @lunny @tboerger