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

add stats guest_count, renamed count to total_count #146

Merged
merged 5 commits into from
May 19, 2021

Conversation

AlexAndBear
Copy link

@AlexAndBear AlexAndBear commented May 18, 2021

Show guest count in stats.

Fixes:

@CLAassistant
Copy link

CLAassistant commented May 18, 2021

CLA assistant check
All committers have signed the CLA.

@AlexAndBear
Copy link
Author

@phil-davis userTypeHelper available since 10.7 how do we handle this?

@AlexAndBear AlexAndBear self-assigned this May 18, 2021
@phil-davis
Copy link
Contributor

phil-davis commented May 18, 2021

@phil-davis userTypeHelper available since 10.7 how do we handle this?

@janackermann it was only complaining about the var not being declared in the class: ae068c1

And the acceptance tests against latest core should work fine, because the latest core release tarball is already 10.7.

I looked at the acceptance tests for 5 seconds - there is only 1 scenario, no interesting test cases there! I guess the unit tests cover checking that the output has the expected stuff.

@AlexAndBear
Copy link
Author

Thank you!

@phil-davis
Copy link
Contributor

Locally tested with:

$ php occ configreport:generate | grep -B 4 -A 8 guest_count
    "stats": {
        "users": {
            "Database": {
                "total_count": 5,
                "guest_count": 0,
                "seen": 5,
                "logged in (30 days)": 1
            }
        },
        "groups": {
            "OC\\Group\\Database": 2
        }
    },

There is a users section with total_count and guest_count.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM - @jvillafanez review please.

@AlexAndBear
Copy link
Author

@phil-davis I will bump min version 10.8 since we need a slight change in the core

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

No workaround for the private class... this will have to do for now.

@phil-davis
Copy link
Contributor

Note: https://drone.owncloud.com/owncloud/configreport/1368/4/3

core master needs to say it is at 10.8 - see PR owncloud/core#38745

@AlexAndBear
Copy link
Author

@phil-davis needed to remove latest temporary from drone.star, agree?

@phil-davis
Copy link
Contributor

phil-davis commented May 19, 2021

@phil-davis needed to remove latest temporary from drone.star, agree?

agree - we know that this new configreport code does not work against latest = 10.7.0

@sonarcloud
Copy link

sonarcloud bot commented May 19, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AlexAndBear AlexAndBear merged commit 0e42938 into master May 19, 2021
@delete-merged-branch delete-merged-branch bot deleted the enterprise/issues/3467 branch May 19, 2021 07:45
@jnweiger jnweiger mentioned this pull request Mar 21, 2022
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants