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

5041 total user statistic shouldnt include closed accounts #5351

Conversation

@MarcBurt
Copy link
Contributor

commented Oct 16, 2014

I'm new to this, sorry if I've done anything wrong!
Saw this: #5041 and attempted to fix it, put the logic in the model rather than the presenter.
The query searches for users with a username (to exclude invitations - mentioned in the thread) and Users with closed accounts.

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Oct 16, 2014

Thanks! That's a nice improvement.

end

it "returns total_users excluding closed accounts & users without usernames" do
expect(User.total_users.count).to eq 5 e #5 users from fixtures

This comment has been minimized.

Copy link
@jhass

jhass Nov 9, 2014

Member

There's a stray e here making the specs fail

@jhass

This comment has been minimized.

Copy link
Member

commented Nov 9, 2014

Please try the following to condense your commits into one:

git remote add upstream git://github.com/diaspora/diaspora.git
git fetch upstream
git checkout 5041-Total-user-statistic-shouldnt-include-closed-accounts
git rebase -i upstream/develop
# Keep pick for the first line, choose squash or fix for all others
# You'll be in a merge conflict now, git status will tell you the files with conflict markers
# When you resolved a conflict mark it as so with git add path/to/file
# When all conflicts are resolved run git rebase --continue
# Watch the output, the process might repeat
#Finally
git push -f origin 5041-Total-user-statistic-shouldnt-include-closed-accounts
@jhass

This comment has been minimized.

Copy link
Member

commented Nov 29, 2014

Hey, are you still interested in getting this in? :)

@MarcBurt MarcBurt force-pushed the MarcBurt:5041-Total-user-statistic-shouldnt-include-closed-accounts branch from 2d2bac2 to 16fa019 Dec 1, 2014

@MarcBurt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2014

Hi,

Sorry - completely forgot, ran through those steps.. Is everything ok now?

On 29 November 2014 at 15:28, Jonne Haß notifications@github.com wrote:

Hey, are you still interested in getting this in? :)


Reply to this email directly or view it on GitHub
#5351 (comment).

@MarcBurt MarcBurt force-pushed the MarcBurt:5041-Total-user-statistic-shouldnt-include-closed-accounts branch from 16fa019 to a57aa27 Dec 1, 2014

@@ -90,6 +90,7 @@ This is disabled by default since it requires the installation of additional pac
* Show error message on invalid reset password attempt [#5325](https://github.com/diaspora/diaspora/pull/5325)
* Fix translations on mobile password reset pages [#5318](https://github.com/diaspora/diaspora/pull/5318)
* Handle unset user agent when signing out [#5316](https://github.com/diaspora/diaspora/pull/5316)
<<<<<<< HEAD

This comment has been minimized.

Copy link
@Flaburgan

Flaburgan Dec 1, 2014

Member

This should not be there ;)

@MarcBurt MarcBurt force-pushed the MarcBurt:5041-Total-user-statistic-shouldnt-include-closed-accounts branch from a57aa27 to 4d9b07e Dec 1, 2014

@MarcBurt

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2014

I think that's solved the issues

@jhass

This comment has been minimized.

Copy link
Member

commented Dec 1, 2014

Looks good, thank you!

@jhass jhass added this to the next-major milestone Dec 1, 2014


it "returns total_users excluding closed accounts & users without usernames" do
expect(User.total_users.count).to eq 5 #5 users from fixtures
end

This comment has been minimized.

Copy link
@jhass

jhass Dec 1, 2014

Member

Ah, tricky. Your nesting is of here, the describe block begins at a different level than the previous it block, causing you to miss an end.

Remember, you can update the commit with git commit --amend and update the pull request with git push -f origin 5041-Total-user-statistic-shouldnt-include-closed-accounts

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 27, 2015

I'd still like this in for the spec, even though the actual fix is already applied. Would you mind fixing above comment and rebasing? :)

@MarcBurt

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2015

Hi, yes I'll be able to do so.
I won't have access to my dev machine for a while however.
On 27 Jan 2015 17:26, "Jonne Haß" notifications@github.com wrote:

I'd still like this in for the spec, even though the actual fix is already
applied. Would you mind rebasing? :)


Reply to this email directly or view it on GitHub
#5351 (comment).

@MarcBurt MarcBurt force-pushed the MarcBurt:5041-Total-user-statistic-shouldnt-include-closed-accounts branch from 4d9b07e to 9f5131b Feb 8, 2015

@jhass jhass merged commit 9f5131b into diaspora:develop Feb 27, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

jhass added a commit that referenced this pull request Feb 27, 2015

Merge pull request #5351 from MarcBurt/5041-Total-user-statistic-shou…
…ldnt-include-closed-accounts

5041 total user statistic shouldnt include closed accounts
@jhass

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

I took the liberty to rebase this and refactor it a bit, thanks!

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

I merged that but the total_user count didn't change on https://diaspora-fr.org/statistics.json

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

Yes, the actual change was already included in #5464, I just backported the spec for it from this PR, where the line in #5464 was also taken from.

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