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

feature: Store the username in eventlog for any entries created through the Webui #6032

Merged
merged 7 commits into from Mar 11, 2017

Conversation

Projects
None yet
7 participants
@towster
Contributor

towster commented Feb 27, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 27, 2017

Thank you for submitting a PR @towster! We have found the following @murrant, @laf and @zarya based on the history of these files to review this PR.

Thank you for submitting a PR @towster! We have found the following @murrant, @laf and @zarya based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 27, 2017

Member

Great idea - I'd personally say we should add another column and record this properly. @librenms/reviewers thoughts?

Member

laf commented Feb 27, 2017

Great idea - I'd personally say we should add another column and record this properly. @librenms/reviewers thoughts?

@geordish

This comment has been minimized.

Show comment
Hide comment
@geordish

geordish Feb 28, 2017

Contributor

Great idea, but I agree with @laf. A column on the event to relate it back to a person would be much better.

Contributor

geordish commented Feb 28, 2017

Great idea, but I agree with @laf. A column on the event to relate it back to a person would be much better.

@laf laf added the Schema label Mar 1, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 1, 2017

Member

I've updated this now to store username. Chose not to do user_id due to varying auth methods and how inconsistence this could be. @librenms/reviewers can someone review pls.

Member

laf commented Mar 1, 2017

I've updated this now to store username. Chose not to do user_id due to varying auth methods and how inconsistence this could be. @librenms/reviewers can someone review pls.

@laf laf changed the title from Added identification of user who removed a device to eventlog entry to feature: Store the username in eventlog for any entries created through the Webui Mar 1, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@towster

This comment has been minimized.

Show comment
Hide comment
@towster

towster Mar 1, 2017

Contributor

Thanks for doing this more properly laf

Contributor

towster commented Mar 1, 2017

Thanks for doing this more properly laf

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 1, 2017

Member

No worries, thanks for prompting us into it :)

Member

laf commented Mar 1, 2017

No worries, thanks for prompting us into it :)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 3, 2017

Member

Updated schema file.

Member

laf commented Mar 3, 2017

Updated schema file.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated html/includes/table/eventlog.inc.php
@@ -60,12 +60,16 @@
$type = $eventlog['type'];
}
$severity_colour = $eventlog['severity'];
if ($eventlog['username'] === 0) {

This comment has been minimized.

@murrant

murrant Mar 8, 2017

Member

Are you sure the identical comparison (===) is the correct one here?
Aren't all db results strings?

Also, out of curiosity, why the check in the first place?

@murrant

murrant Mar 8, 2017

Member

Are you sure the identical comparison (===) is the correct one here?
Aren't all db results strings?

Also, out of curiosity, why the check in the first place?

Show outdated Hide outdated includes/functions.php
@@ -830,13 +830,19 @@ function log_event($text, $device = null, $type = null, $severity = 2, $referenc
$device = device_by_id_cache($device);
}
$username = '';
if (function_exists(get_userid) && !empty($_SESSION['username'])) {

This comment has been minimized.

@murrant

murrant Mar 8, 2017

Member

function_exists() a left over?

If so, why check if $_SESSION['username'] is not empty, only to set it as '' if it is empty?

@murrant

murrant Mar 8, 2017

Member

function_exists() a left over?

If so, why check if $_SESSION['username'] is not empty, only to set it as '' if it is empty?

This comment has been minimized.

@laf

laf Mar 8, 2017

Member

Yeah I had userid as the one being stored originally.

Updated

@laf

laf Mar 8, 2017

Member

Yeah I had userid as the one being stored originally.

Updated

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Mar 8, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@murrant murrant merged commit 7bb9d58 into librenms:master Mar 11, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment