Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

WIP: add hasbeenactive #43370

Closed
wants to merge 7 commits into from
Closed

WIP: add hasbeenactive #43370

wants to merge 7 commits into from

Conversation

mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Oct 24, 2022

This PR is to add a sorting flag for returning users that have a null value for lastActiveTime or not. This user state may be caused by a user having never been active on the Sourcegraph instance, or having not been active for 93 days (this is the length of time entries in the event_logs table are persisted in the database)

Test plan

Unit Tests

…ing of includeNeverActive

I'd like the default behavior to be -- without this filter never active users are not returned by the inactiveSince query

left off working on unit tests -- incomplete state
Copy link
Contributor

@efritz efritz left a comment

Choose a reason for hiding this comment

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

Talked in a hack session about the desired semantics. Here are a few tips to get you further towards this. This should make the test easier to write as well now that we've clarified the misunderstanding.

cmd/frontend/graphqlbackend/schema.graphql Outdated Show resolved Hide resolved
cmd/frontend/graphqlbackend/users.go Outdated Show resolved Hide resolved
internal/database/users.go Outdated Show resolved Hide resolved
internal/database/users.go Outdated Show resolved Hide resolved
Comment on lines 811 to 812
// NeverActive filters out users that have never had an eventlog entry if true.
NeverActive bool
Copy link
Contributor

@efritz efritz Nov 4, 2022

Choose a reason for hiding this comment

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

  • Rename
  • Redocument to match graphql layer semantics

…ter --

TODO: create new unit Tests for both TestUsers_InactiveSince & TestUsers_IncludeNeverActive
@@ -34,6 +35,9 @@ func (r *schemaResolver) Users(args *usersArgs) *userConnectionResolver {
if args.InactiveSince != nil {
opt.InactiveSince = args.InactiveSince.Time
}
if args.IncludeNeverActive != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should also be nested under if args.InactiveSince != nil {

cmd/frontend/graphqlbackend/schema.graphql Outdated Show resolved Hide resolved
@DaedalusG
Copy link
Contributor

After exploring this a bit more I will need to review the logic and rename some variables that are named never active -- this is in light of the fact that users may have lastActiveTime null due to all their event_logs entries expiring after 93 days of inactivity

I still think having a filter here is going to come in handy though, in src users clean I'll move to using the last_active_at db entry -- more context here

cc: @mrnugget @efritz

@DaedalusG DaedalusG marked this pull request as ready for review December 1, 2022 01:08
@DaedalusG
Copy link
Contributor

DaedalusG commented Dec 15, 2022

Closing in light of: sourcegraph/src-cli#901

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants