Skip to content

Fix missing updated time on migrated issues and comments#9744

Merged
zeripath merged 3 commits into
go-gitea:masterfrom
lunny:lunny/fix_migrate_comment
Jan 14, 2020
Merged

Fix missing updated time on migrated issues and comments#9744
zeripath merged 3 commits into
go-gitea:masterfrom
lunny:lunny/fix_migrate_comment

Conversation

@lunny
Copy link
Copy Markdown
Member

@lunny lunny commented Jan 13, 2020

Should fix #9741

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 13, 2020
@lunny lunny modified the milestones: 1.11.0, 1.12.0 Jan 13, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 13, 2020
sapk
sapk previously approved these changes Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

Need adjustement on test data otherwise LGTM

@GiteaBot GiteaBot 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 Jan 13, 2020
@6543
Copy link
Copy Markdown
Member

6543 commented Jan 13, 2020

@lunny found second bug:
func (g *GithubDownloaderV3) GetPullRequests(page, perPage int) ([]*base.PullRequest, error) {
need a patch too around Line 525 link

6543
6543 requested changes Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

as comment

@6543
Copy link
Copy Markdown
Member

6543 commented Jan 13, 2020

@sapk no there is a missing line ...

@6543

This comment has been minimized.

Sign in to view
@sapk sapk dismissed their stale review January 13, 2020 16:00

seems to need more fix

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 13, 2020
@sapk sapk removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jan 13, 2020
@lunny lunny force-pushed the lunny/fix_migrate_comment branch from b9eefdf to b00a6bc Compare January 14, 2020 00:04
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 14, 2020

Codecov Report

Merging #9744 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9744      +/-   ##
=========================================
+ Coverage    42.3%   42.3%   +<.01%     
=========================================
  Files         598     598              
  Lines       78318   78324       +6     
=========================================
+ Hits        33135   33138       +3     
- Misses      41127   41130       +3     
  Partials     4056    4056
Impacted Files Coverage Δ
modules/migrations/base/pullrequest.go 0% <ø> (ø) ⬆️
modules/migrations/gitea.go 7.91% <0%> (-0.05%) ⬇️
modules/migrations/github.go 77.46% <100%> (+0.15%) ⬆️
services/pull/check.go 48.64% <0%> (-4.73%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/pull.go 42.47% <0%> (+0.6%) ⬆️
models/webhook.go 70.46% <0%> (+1.06%) ⬆️
modules/queue/workerpool.go 41.2% <0%> (+1.28%) ⬆️

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 1751d5f...b00a6bc. Read the comment docs.

@techknowlogick
Copy link
Copy Markdown
Member

techknowlogick commented Jan 14, 2020

Is a new migration needed for these changes (not possible for backport, but what about master)?

@6543
Copy link
Copy Markdown
Member

6543 commented Jan 14, 2020

migration is only needed if we like to fix old records

@techknowlogick
Copy link
Copy Markdown
Member

techknowlogick commented Jan 14, 2020

@6543 but this is new column

@lunny
Copy link
Copy Markdown
Member Author

lunny commented Jan 14, 2020

@techknowlogick will add a migration.

@GiteaBot GiteaBot 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 Jan 14, 2020
@6543
Copy link
Copy Markdown
Member

6543 commented Jan 14, 2020

@techknowlogick colum already exist in db. But with wrong entrys for migrated issues ...

@techknowlogick
Copy link
Copy Markdown
Member

techknowlogick commented Jan 14, 2020

@6543 ah ok. Thanks for pointing that out. I thought this was a different struct.

@zeripath
Copy link
Copy Markdown
Contributor

zeripath commented Jan 14, 2020

Make lg-tm work

@zeripath zeripath merged commit 7f869c0 into go-gitea:master Jan 14, 2020
@lunny lunny deleted the lunny/fix_migrate_comment branch January 14, 2020 16:21
lunny added a commit to lunny/gitea that referenced this pull request Jan 14, 2020
@lunny @sapk
Fix missing updated time on migrated issues and comments (go-gitea#9744)
f55da38 
* Fix missing updated time on migrated issues and comments

* Fix testing and missing updated on migrating pullrequest

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
@lunny lunny mentioned this pull request Jan 14, 2020
Merged
@lunny lunny added the backport/done All backports for this PR have been created label Jan 14, 2020
sapk added a commit that referenced this pull request Jan 14, 2020
@lunny @sapk
Fix missing updated time on migrated issues and comments (#9744) (#9764)
2850879 
* Fix missing updated time on migrated issues and comments

* Fix testing and missing updated on migrating pullrequest

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@6543 6543 6543 approved these changes

+2 more reviewers

@guillep2k guillep2k guillep2k approved these changes

@sapk sapk sapk left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug

Projects

None yet

Milestone

1.12.0

Development

Successfully merging this pull request may close these issues.

Wrong meta info at issueComments on github migration

8 participants

@lunny @6543 @codecov-io @techknowlogick @zeripath @sapk @guillep2k @GiteaBot