Skip to content
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

Fix milestone num_issues #8221

Merged
merged 11 commits into from Oct 6, 2019

Conversation

@lunny
Copy link
Member

commented Sep 18, 2019

Should fix #8190

@lunny lunny added the kind/bug label Sep 18, 2019
@lunny lunny added this to the 1.10.0 milestone Sep 18, 2019
@lunny lunny added the backport/v1.9 label Sep 18, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Sep 18, 2019

Codecov Report

Merging #8221 into master will increase coverage by <.01%.
The diff coverage is 51.35%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #8221      +/-   ##
=========================================
+ Coverage   41.79%   41.8%   +<.01%     
=========================================
  Files         497     497              
  Lines       65603   65606       +3     
=========================================
+ Hits        27422   27424       +2     
- Misses      34664   34666       +2     
+ Partials     3517    3516       -1
Impacted Files Coverage Δ
models/issue.go 50.71% <0%> (ø) ⬆️
models/issue_milestone.go 60.19% <54.28%> (+0.39%) ⬆️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
routers/repo/view.go 42.53% <0%> (-1.02%) ⬇️
models/gpg_key.go 56.66% <0%> (+0.83%) ⬆️
models/repo_list.go 74.14% <0%> (+0.97%) ⬆️

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 bc5a479...c80a64f. Read the comment docs.

}

if err = updateMilestone(e, m); err != nil {
if err := updateMilestoneClosedNum(e, oldMilestoneID); err != nil {

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 18, 2019

Member

It looks like this will execute always, whether the issue closed status has changed or not.

This comment has been minimized.

Copy link
@lunny

lunny Sep 18, 2019

Author Member

No. The function changeMilestoneAssign will only be invoked when issues/pulls status changed.

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 18, 2019

Member

Isn't changeMilestoneAssign() used to move the target milestone of the issue? (e.g. 1.10.0 --> 1.11.0).
I've just set a breakpoint on the function and set a new milestone for one issue. The function was definitely called, and the issue never changed its IsClosed status.

This comment has been minimized.

Copy link
@lunny

lunny Sep 18, 2019

Author Member

see line 336, this will be invoked only if oldMilestoneID > 0 {

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 18, 2019

Member

oldMilestoneID > 0 != isClosed
I'm probably misunderstanding something but what I mean is:

  • Milestone 1.10.0 has ID: 4; milestone 1.11.0 has ID: 5.
  • Create issue #123; assign to milestone 1.10.0 (issue.MilestoneID=4).
    • 1.10.0 ==> num_issues = 1, num_closed_issues = 0
    • 1.11.0 ==> num_issues = 0, num_closed_issues = 0
  • Edit issue #123; change milestone from 1.10.0 to 1.11.0.

Now changeMilestoneAssign() is called with issue.MilestoneID=5, oldMilestoneID = 4, and issue.IsClosed = false. We expect:

  • 1.10.0 ==> num_issues = 0, num_closed_issues = 0
  • 1.11.0 ==> num_issues = 1, num_closed_issues = 0

In no case num_closed_issues should be updated, but oldMilestoneID > 0 and updateMilestoneClosedNum() is called.

}

if err = updateMilestone(e, m); err != nil {
if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 18, 2019

Member

Same here. If the issue is not closed, I think this function should not be called.

This comment has been minimized.

Copy link
@lunny

lunny Sep 18, 2019

Author Member

As above.

This comment has been minimized.

Copy link
@lunny

lunny Sep 18, 2019

Author Member

This one is different from previous one. This function is changeStatus which will be invoked when issue is closed or reopened.

@lunny

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

Forgot update completeness.

@lunny lunny force-pushed the lunny:lunny/fix_number branch from a2ca336 to d0b62bb Sep 18, 2019
@lunny lunny removed the status/blocked label Sep 18, 2019
@lunny

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

@guillep2k You are right. Done.

m.NumClosedIssues--
}
func updateMilestoneTotalNum(e Engine, milestoneID int64) (err error) {
_, err = e.Exec("UPDATE `milestone` SET num_issues=(SELECT count(*) FROM issue WHERE milestone_id=?), completeness=if(num_issues>0,100*num_closed_issues/num_issues, 0) WHERE id=?",

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 18, 2019

Member

I'm afraid completeness will lag behind one issue. It may be engine dependent, but i think that num_issues on the right is evaluated before num_issues on the left is updated. completeness is not currently checked in any tests, AFAIK.

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 24, 2019

Member

To solve the division by 0 without using if(), you could use:

UPDATE milestone SET num_issues=(SELECT count(*) FROM issue WHERE milestone_id=?),
completeness = 100*num_closed_issues/(num_issues+1-sign(num_issues)) WHERE id=?;

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 24, 2019

Member

However it does lag one value behind:

; First, a milestone with wrong value in num_issues:
giteatest=> select id, name, num_issues, num_closed_issues, completeness
    from milestone where id = 1;
 id |      name       | num_issues | num_closed_issues | completeness
----+-----------------+------------+-------------------+--------------
  1 | First milestone |          0 |                 1 |           50
(1 row)

; Run the UPDATE for the first time
giteatest=> UPDATE milestone SET num_issues=(SELECT count(*) FROM issue WHERE milestone_id=1),
    completeness = 100*num_closed_issues/(num_issues+1-sign(num_issues)) WHERE id=1;
UPDATE 1

; The UPDATE corrected the value for num_issues, but completeness was calculated using
; the previous value in num_issues
giteatest=> select id, name, num_issues, num_closed_issues, completeness
    from milestone where id = 1;
 id |      name       | num_issues | num_closed_issues | completeness
----+-----------------+------------+-------------------+--------------
  1 | First milestone |          2 |                 1 |          100
(1 row)

; Run the same UPDATE a second time.
giteatest=> UPDATE milestone SET num_issues=(SELECT count(*) FROM issue WHERE milestone_id=1),
    completeness = 100*num_closed_issues/(num_issues+1-sign(num_issues)) WHERE id=1;
UPDATE 1

; Now completeness is calculated correctly
; because num_issues already had the right value.
giteatest=> select id, name, num_issues, num_closed_issues, completeness
    from milestone where id = 1;
 id |      name       | num_issues | num_closed_issues | completeness
----+-----------------+------------+-------------------+--------------
  1 | First milestone |          2 |                 1 |           50
(1 row)

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 24, 2019

Member

I think your best bet is to use an UPDATE JOIN. The following works correctly in every case:

UPDATE milestone
SET num_issues=c.issues, num_closed_issues=c.closed_issues,
    completeness = 100*c.closed_issues/(c.issues+1-sign(c.issues))
FROM (SELECT count(*) as issues,
      SUM(CASE is_closed WHEN true THEN 1 ELSE 0 END) as closed_issues
      FROM issue WHERE milestone_id = ?) as c
WHERE id = ?;

This comment has been minimized.

Copy link
@lunny

lunny Sep 25, 2019

Author Member

@guillep2k But it seems not all databases support the SQL well.

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 25, 2019

Member

Yeah, I see that sqlite3 can't handle that. 😞

@lunny lunny force-pushed the lunny:lunny/fix_number branch from bfb926d to cb2cda5 Sep 19, 2019
@techknowlogick

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

CI Fail

@lunny lunny force-pushed the lunny:lunny/fix_number branch 2 times, most recently from 79b285f to 0c59652 Sep 25, 2019
@lunny

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

It seems sqlite didn't support sign function.

--- FAIL: TestUpdateIssuesCommit (0.01s)
    action_test.go:277: 
        	Error Trace:	action_test.go:277
        	Error:      	Received unexpected error:
        	            	no such function: sign
        	Test:       	TestUpdateIssuesCommit
@guillep2k

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

It seems sqlite didn't support sign function.

--- FAIL: TestUpdateIssuesCommit (0.01s)
    action_test.go:277: 
        	Error Trace:	action_test.go:277
        	Error:      	Received unexpected error:
        	            	no such function: sign
        	Test:       	TestUpdateIssuesCommit

😞 There are alternatives for each engine, but I couldn't find one that works in all of them.

@guillep2k

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

In sqlite there is min(num_issues,1), but that won't work with mssql.

lunny added 6 commits Sep 18, 2019
…ew milestone or clear milestone
@lunny lunny force-pushed the lunny:lunny/fix_number branch from 0c59652 to 912d305 Sep 29, 2019
Copy link
Member

left a comment

Just for clarity. Otherwise LGTM.

}

return updateMilestone(e, m)
_, err = e.Exec("UPDATE `milestone` SET completeness=100*num_closed_issues/(num_issues+1-(CASE WHEN num_issues > 0 THEN 1 ELSE 0 END)) WHERE id=?",

This comment has been minimized.

Copy link
@guillep2k

guillep2k Sep 29, 2019

Member
Suggested change
_, err = e.Exec("UPDATE `milestone` SET completeness=100*num_closed_issues/(num_issues+1-(CASE WHEN num_issues > 0 THEN 1 ELSE 0 END)) WHERE id=?",
_, err = e.Exec("UPDATE `milestone` SET completeness=100*num_closed_issues/(CASE WHEN num_issues > 0 THEN num_issues ELSE 1 END) WHERE id=?",

More clear, perhaps?

This comment has been minimized.

Copy link
@lunny

lunny Sep 29, 2019

Author Member

Good catch. Done.

@GiteaBot GiteaBot removed the lgtm/need 2 label Sep 29, 2019
lunny added 2 commits Sep 30, 2019
lunny and others added 2 commits Oct 5, 2019
@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Oct 6, 2019
@techknowlogick techknowlogick merged commit 51fade4 into go-gitea:master Oct 6, 2019
2 checks passed
2 checks passed
approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@techknowlogick

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

@lunny please send backport :)

@6543

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

@techknowlogick i can create it too?
I dont like to steel @lunny's work but i like to see 1.9.4 released ...

@techknowlogick

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

@6543 sure. If you cherrypick the commit that was merged then @lunny will remain author (worst case a maintainer can rewrite git history of PR before merge)

6543 added a commit to 6543-forks/gitea that referenced this pull request Oct 6, 2019
* fix milestone num_issues

* update missing completeness

* only update milestone closed number when closed issue is assigned a new milestone or clear milestone

* fix tests

* fix update milestone num

* fix completeness calculate

* make completeness calucation more clear
@lunny lunny deleted the lunny:lunny/fix_number branch Oct 7, 2019
lunny added a commit that referenced this pull request Oct 7, 2019
* fix milestone num_issues

* update missing completeness

* only update milestone closed number when closed issue is assigned a new milestone or clear milestone

* fix tests

* fix update milestone num

* fix completeness calculate

* make completeness calucation more clear
@6543

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@lunny add lable backport done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.