Add Activity page to repository #2674

lafriks · 2017-10-07T22:32:01Z

Adds pulse page to repository to display latest activity from issues and pull requests

Screenshot:

codecov-io · 2017-10-07T23:16:21Z

Codecov Report

Merging #2674 into master will decrease coverage by 0.25%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2674      +/-   ##
==========================================
- Coverage   27.38%   27.12%   -0.26%     
==========================================
  Files          86       87       +1     
  Lines       17014    17176     +162     
==========================================
  Hits         4659     4659              
- Misses      11675    11837     +162     
  Partials      680      680

Impacted Files	Coverage Δ
models/repo.go	`13.07% <0%> (-0.1%)`	⬇️
models/repo_activity.go	`0% <0%> (ø)`

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 8863e74...2f44af9. Read the comment docs.

ethantkoenig

With regard to the screenshots, we should try to fix ungrammatical phrases like "1 pull requests" and "1 people".

Also, would "users" be a better word choice than "people"?

ethantkoenig · 2017-10-08T05:05:50Z

models/repo_pulse.go

+	sess := x.Where("pull_request.base_repo_id=?", baseRepoID).
+		Join("INNER", "issue", "pull_request.issue_id = issue.id")
+
+	var mergedCond, createdCond builder.Cond


I believe this can be simplified

var cond builder.Cond if merged { cond = cond.Or(...) } if created { cond = cond.Or(...) } sess.And(cond)

ethantkoenig · 2017-10-08T05:09:02Z

routers/repo/pulse.go

+		timeFrom = timeTill.Add(-time.Hour * 168)
+	}
+	ctx.Data["DateFrom"] = timeFrom.Format("January 2, 2006")
+	ctx.Data["DateTill"] = timeTill.Format("January 2, 2006")


nit: please use "DateUntil"; it is not substanially longer, and is more explicit

ethantkoenig · 2017-10-08T05:10:40Z

routers/repo/pulse.go

+
+	if ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) {
+		if err := models.FillPullRequestsForPulse(stats, ctx.Repo.Repository.ID, timeFrom); err != nil {
+			ctx.Error(500, "FillPullRequestsForPulse: "+err.Error())


I believe ctx.Status(500, "Function", err) is preferred, and is used everywhere else

ethantkoenig · 2017-10-08T05:12:02Z

routers/routes/routes.go

@@ -639,6 +644,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 		m.Get("/compare/:before([a-z0-9]{40})\\.\\.\\.:after([a-z0-9]{40})", repo.SetEditorconfigIfExists,
 			repo.SetDiffViewStyle, repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode), repo.CompareDiff)
 	}, ignSignIn, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits())
+


nit: please remove

bkcsoft · 2017-10-09T21:47:18Z

Could we call it "Activity" instead? (I never understood they GitHub called it "Pulse" 😂 )

lafriks · 2017-10-12T22:05:27Z

@bkcsoft renamed tab to "Activity"
@ethantkoenig done all your requested code changes

As bonus added published releases info

Updated screenshot:

lunny · 2017-10-13T06:34:56Z

routers/repo/activity.go

+package repo
+
+import (
+	//"fmt"


Just remove the comment imports?

lunny · 2017-10-13T06:36:36Z

routers/routes/routes.go

+		m.Group("/activity", func() {
+			m.Get("", repo.Activity)
+			m.Get("/:period", repo.Activity)
+		}, context.RepoRef(), repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode))


This is a sub feature of unit Code?

I was thinking at first it should be code but now I don't know actually... What do you think?

I think it should be available to users who have permission to view PR and Issues.

@lafriks I agree with @daviian, if you have Issues or Pull Request you could visit the activity.

lafriks · 2017-10-13T19:47:57Z

@lunny cleaned up forgotten imports and changed unit check to only needed ones

lunny · 2017-10-14T03:12:42Z

LGTM

lunny · 2017-10-14T03:16:38Z

models/repo_activity.go

+
+	// Published releases authors
+	sess = releasesForActivityStatement(baseRepoID, fromTime)
+	if _, err = sess.Select("count(distinct release.publisher_id) as `count`").Table("release").Get(row); err != nil {


var count int64 sess.Select().Table().Get(&count)

The current vendored xorm already allowed you do that.

lunny · 2017-10-14T03:18:09Z

One more improvement.

ethantkoenig · 2017-10-13T02:44:58Z

routers/repo/activity.go

+	if err := models.FillUnresolvedIssuesForActivity(stats, ctx.Repo.Repository.ID, timeFrom,
+		ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues),
+		ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests)); err != nil {
+		ctx.Handle(500, "FillIssuesForActivity", err)


wrong error message

ethantkoenig · 2017-10-13T02:45:49Z

routers/repo/activity.go

+package repo
+
+import (
+	//"fmt"


Please remove all of these commented-out imports

ethantkoenig · 2017-10-14T06:21:54Z

modules/templates/helper.go

+	}
+
+	// Special rules for Latvian language
+	if lang == "lv-LV" {


Maybe outside scope of this PR, but...

I'm not sure having hard-coded language-specific rules in the code is the right approach. I think ideally that language-specific rules should be expressed in that language's locale file, not in code. Two reasons for this:

Many gitea translators are not familiar with the codebase

Even hard-coded rules like this one may not be enough to support all languages. For instance, Arabic has three grammatical numbers (singular, dual, plural), so a function taking key1 and keyN arguments wouldn't work for Arabic, regardless of what hard-coded rules we add.

Of course, encoding language-specific rule in the locale may require a more expressive i18n scheme, but that seems like the more sustainable approach.

@ethantkoenig I don't see how such rule could be implemented in locale file. That would require to implement math parser in GoLang so that such rules could be used and this is totally out of scope for this PR

Morlinest · 2017-10-14T13:54:45Z

models/repo.go

@@ -392,6 +392,19 @@ func (repo *Repository) UnitEnabled(tp UnitType) bool {
 	return false
 }

+// AnyUnitEnabled if this repository has the any of the given units enabled
+func (repo *Repository) AnyUnitEnabled(tps ...UnitType) bool {
+	repo.getUnits(x)


There is also getUnitsByRepoIDAndIDs (currently unused anywhere)

As we already discussed in discord it is better to keep same behaviour as in UnitEnabled

Morlinest · 2017-10-14T14:01:32Z

Shouldn't this feature be new Unit?

lafriks · 2017-10-14T14:12:25Z

@Morlinest Units are like right units. So they are not same as tabs, so no new unit is not needed for this

Morlinest · 2017-10-14T14:19:56Z

@lafriks Right, I was thinking about option to disable/enable it. But there's probably no reason to do that.

ethantkoenig · 2017-10-14T17:18:04Z

models/repo.go

@@ -392,6 +392,19 @@ func (repo *Repository) UnitEnabled(tp UnitType) bool {
 	return false
 }

+// AnyUnitEnabled if this repository has the any of the given units enabled
+func (repo *Repository) AnyUnitEnabled(tps ...UnitType) bool {
+	repo.getUnits(x)


Unchecked error returned by repo.getUnits(x)

I'm keeping same behaviour as in CheckUnit :) I will see if without much of rewriting there is anything I can do about it

@ethantkoenig as this needs to be used in templates so it can not return error so I don't see much I can do here and it is same as it is in UnitEnabled method already

Can we at least log it?

* Rename pulse to activity * Implement correct count wording * Code cleanup

lafriks · 2017-10-14T22:52:50Z

@ethantkoenig I did rewrite plural text translation method a bit to easier support multiple language exceptions but that is all that can be done currently. My native Latvian language also has 3 forms (singular for numbers ending with 1 except if ending with 11, plural and special case for 0) but without completely changing i18n library that is built into go-macaron framework gitea uses there is no way we can support that...

ethantkoenig · 2017-10-14T22:58:43Z

@lafriks Okay, definitely an improvement.

To be clear, I didn't mean that this PR be should blocked on the translation issue, but was just trying to make a general comment about a long-term possibility; sorry if that wasn't clear. And yes, I realize that we would need to update the i18n library to do what I was suggesting.

lafriks · 2017-10-14T23:07:26Z

@ethantkoenig added repo unit error logging

ethantkoenig · 2017-10-14T23:12:56Z

LGTM

ethantkoenig · 2017-10-14T23:14:19Z

One other thing: could we add a test?

lafriks · 2017-10-14T23:27:29Z

@ethantkoenig I thought about it but only test I can add if page opens without errors as stats are depending on time and preparing data for that would be quite hard

lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Oct 7, 2017

lafriks added this to the 1.3.0 milestone Oct 7, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 7, 2017

ethantkoenig suggested changes Oct 8, 2017

View reviewed changes

lafriks force-pushed the feat/pulse branch from 36acace to fb10009 Compare October 12, 2017 22:02

lafriks changed the title ~~Add Pulse page to repository~~ Add Activity page to repository Oct 12, 2017

lunny requested changes Oct 13, 2017

View reviewed changes

lunny approved these changes Oct 14, 2017

View reviewed changes

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 Oct 14, 2017

lunny reviewed Oct 14, 2017

View reviewed changes

ethantkoenig suggested changes Oct 14, 2017

View reviewed changes

Morlinest reviewed Oct 14, 2017

View reviewed changes

ethantkoenig suggested changes Oct 14, 2017

View reviewed changes

lafriks added 7 commits October 15, 2017 00:52

Add Pulse tab to repisotory page

5de8a82

Pull request data for pulse

753f34b

Add issue stats data and pulse page view

b352580

Add user unit right checks

4842575

Add releases to activity

0996e13

* Rename pulse to activity * Implement correct count wording * Code cleanup

Remove unneded newline

1c48cc6

Fix right checking to only needed ones

cb22128

Refactor count selection to not use struct but just integer

a15d624

lafriks force-pushed the feat/pulse branch from 50ea0bb to a15d624 Compare October 14, 2017 22:00

Improve plural text translation function

145c509

Fix formatting

81ad114

Log repository unit loading error

2f44af9

lafriks force-pushed the feat/pulse branch from f62a7c6 to 2f44af9 Compare October 14, 2017 23:08

ethantkoenig approved these changes Oct 14, 2017

View reviewed changes

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 Oct 14, 2017

lafriks merged commit f42dbdb into go-gitea:master Oct 14, 2017

lafriks deleted the feat/pulse branch October 14, 2017 23:17

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Add Activity page to repository #2674

Add Activity page to repository #2674

Conversation

lafriks commented Oct 7, 2017

codecov-io commented Oct 7, 2017 • edited Loading

Codecov Report

ethantkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Oct 9, 2017

lafriks commented Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Oct 13, 2017

lunny commented Oct 14, 2017

Choose a reason for hiding this comment

lunny commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morlinest commented Oct 14, 2017

lafriks commented Oct 14, 2017

Morlinest commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Oct 14, 2017

ethantkoenig commented Oct 14, 2017

lafriks commented Oct 14, 2017

ethantkoenig commented Oct 14, 2017

ethantkoenig commented Oct 14, 2017 • edited Loading

lafriks commented Oct 14, 2017

codecov-io commented Oct 7, 2017 •

edited

Loading

lafriks commented Oct 12, 2017 •

edited

Loading

ethantkoenig commented Oct 14, 2017 •

edited

Loading