Hide unactive on explore users and some refactors #2741

lunny · 2017-10-19T07:13:47Z

Unactive user should not displayed on explore UI. This PR also reafctors SearchUserName to SearchUsers.

codecov-io · 2017-10-19T08:13:07Z

Codecov Report

Merging #2741 into master will increase coverage by 0.17%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
+ Coverage    26.9%   27.08%   +0.17%     
==========================================
  Files          88       88              
  Lines       17289    17264      -25     
==========================================
+ Hits         4652     4676      +24     
+ Misses      11958    11909      -49     
  Partials      679      679

Impacted Files	Coverage Δ
models/org.go	`68.94% <ø> (-0.87%)`	⬇️
models/user.go	`21.45% <92.3%> (+3.21%)`	⬆️
models/issue_watch.go	`66.66% <0%> (-17.55%)`	⬇️
models/notification.go	`69.35% <0%> (+10.75%)`	⬆️

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 0390030...21cadbd. Read the comment docs.

ethantkoenig

A few minor things

ethantkoenig · 2017-10-21T21:33:35Z

models/org_test.go

@@ -240,7 +240,8 @@ func TestCountOrganizations(t *testing.T) {
 func TestOrganizations(t *testing.T) {


This test should be moved from org_test to user_test

This test will only set type = UserTypeOrganization, it's for organizations but not users. Since Users and Organizations share the user table. SearchUsers will be shared also.

But it tests a function that is defined in user.go, whereas it previously tested a function that was defined in org.go. It is my understanding that if something is defined in XYZ.go, its test should be in XYZ_test.go; is my understanding incorrect?

But should I rename it from TestOrganizations to TestUsers or just put TestOrganizations to user_test.go? This function logically test organizations but SearchUsers is shared by Users and Organizations although it's in user.go. So I don't think we can obey the rule you mentioned above.

I would suggest renaming it to TestSearchUsers, and expanding it to test for both individual users and organizations, but you can do what you think is best.

One way or another, however, we should make sure that the SearchUsers function is fully tested (both individual users and organizations).

I will send another commit to do that.

ethantkoenig · 2017-10-21T21:36:07Z

models/user.go

+	IsActive util.OptionalBool
+}
+
+func (opts *SearchUserOptions) toConds() builder.Cond {


nit: since this returns a Cond, please call it toCond().

@ethantkoenig For SearchUserOptions contains not only 1 condition it should be toConds. And builder.Cond maybe one condition or composite conditions, so toConds will be more suitable.

lunny · 2017-10-22T04:46:22Z

@ethantkoenig done.

ethantkoenig

Some nits, but looks good otherwise. +1 for adding the methods to OptionalBool

ethantkoenig · 2017-10-22T05:13:02Z

models/notification_test.go

+	// 9 are inactive, thus notifications for user 1 and 4 is created
+	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification)
+	assert.Equal(t, NotificationStatusUnread, notf.Status)
+	CheckConsistencyFor(t, &Issue{ID: issue.ID})


nit: don't need to call CheckConsistencyFor(t, &Issue{ID: issue.ID}) twice, since nothing changes in between the calls. Preferably, only call at the end of the test, for consistency with other tests

ethantkoenig · 2017-10-22T05:15:03Z

modules/util/util.go

+	return o == OptionalBoolFalse
+}
+
+// IsNone return true if equal to OptionalBoolFalse


OptionalBoolFalse -> OptionalBoolNone

ethantkoenig · 2017-10-22T05:17:33Z

models/notification_test.go

@@ -16,8 +16,12 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {

 	assert.NoError(t, CreateOrUpdateIssueNotifications(issue, 2))

-	// Two watchers are inactive, thus only notification for user 10 is created
-	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 10, IssueID: issue.ID}).(*Notification)
+	// 9 are inactive, thus notifications for user 1 and 4 is created


nit: change to // User 9 is inactive, .... The current version could mistakenly be interpreted as "9 users are inactive".

ethantkoenig · 2017-10-22T05:18:16Z

LGTM

appleboy · 2017-10-24T15:17:18Z

LGTM

* hide unactive on explore users and some refactors * fix test for removed Organizations * fix test for removed Organizations * fix imports * fix logic bug * refactor the toConds * Rename TestOrganizations to TestSearchUsers and add tests for users * fix other tests * fix other tests * fix watchers tests * fix comments and remove unused code

lunny added type/enhancement An improvement of existing functionality type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Oct 19, 2017

lunny force-pushed the lunny/hide_unactive_user_explore branch from 7ad18cd to 884fdcd Compare October 19, 2017 07:18

lunny added this to the 1.3.0 milestone Oct 19, 2017

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

lunny added type/bug and removed type/enhancement An improvement of existing functionality labels Oct 19, 2017

ethantkoenig suggested changes Oct 21, 2017

View reviewed changes

lunny added 6 commits October 22, 2017 10:20

hide unactive on explore users and some refactors

4ee313a

fix test for removed Organizations

03cd6e5

fix test for removed Organizations

1dacbcd

fix imports

7fc4792

fix logic bug

623f897

refactor the toConds

c849eab

lunny force-pushed the lunny/hide_unactive_user_explore branch from f27202c to c849eab Compare October 22, 2017 02:21

lunny added 4 commits October 22, 2017 11:17

Rename TestOrganizations to TestSearchUsers and add tests for users

8302855

fix other tests

97d7d6d

fix other tests

b6f81f8

fix watchers tests

f7db974

ethantkoenig approved these changes Oct 22, 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 22, 2017

fix comments and remove unused code

2e38a7e

appleboy approved these changes Oct 24, 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 24, 2017

Merge branch 'master' into lunny/hide_unactive_user_explore

21cadbd

lafriks merged commit 6eeadb2 into go-gitea:master Oct 24, 2017

lunny deleted the lunny/hide_unactive_user_explore branch October 25, 2017 00:34

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide unactive on explore users and some refactors #2741

Hide unactive on explore users and some refactors #2741

lunny commented Oct 19, 2017

codecov-io commented Oct 19, 2017 •

edited

Loading

ethantkoenig left a comment

ethantkoenig Oct 21, 2017

lunny Oct 22, 2017

ethantkoenig Oct 22, 2017

lunny Oct 22, 2017 •

edited

Loading

ethantkoenig Oct 22, 2017

lunny Oct 22, 2017

ethantkoenig Oct 21, 2017

lunny Oct 22, 2017

lunny commented Oct 22, 2017

ethantkoenig left a comment •

edited

Loading

ethantkoenig Oct 22, 2017 •

edited

Loading

lunny Oct 22, 2017

ethantkoenig Oct 22, 2017

lunny Oct 22, 2017

ethantkoenig Oct 22, 2017

lunny Oct 22, 2017

ethantkoenig commented Oct 22, 2017

appleboy commented Oct 24, 2017

		@@ -240,7 +240,8 @@ func TestCountOrganizations(t *testing.T) {
		func TestOrganizations(t *testing.T) {

Hide unactive on explore users and some refactors #2741

Hide unactive on explore users and some refactors #2741

Conversation

lunny commented Oct 19, 2017

codecov-io commented Oct 19, 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

lunny Oct 22, 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

lunny commented Oct 22, 2017

ethantkoenig left a comment • edited Loading

Choose a reason for hiding this comment

ethantkoenig Oct 22, 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

Choose a reason for hiding this comment

ethantkoenig commented Oct 22, 2017

appleboy commented Oct 24, 2017

codecov-io commented Oct 19, 2017 •

edited

Loading

lunny Oct 22, 2017 •

edited

Loading

ethantkoenig left a comment •

edited

Loading

ethantkoenig Oct 22, 2017 •

edited

Loading