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.

Sign up for GitHub

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

Jump to bottom

Add refresh in widget settings #12127

Merged
merged 11 commits into from
Oct 2, 2020
Merged

Add refresh in widget settings #12127

merged 11 commits into from
Oct 2, 2020

Conversation

Negatifff
Copy link
Contributor

Please give a short description what your pull request is for

DO NOT DELETE THE UNDERLYING TEXT

Please note

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

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@Negatifff
Copy link
Contributor Author

Negatifff commented Sep 21, 2020 •
edited
Loading

and need db correction:

alter table users_widgets modify column 'refresh' smallint(4) NOT NULL DEFAULT '60';

but I don't know how do this in patch.

and I understand that perhaps the solution is not the most beautiful...

@Negatifff Negatifff changed the title Timeout in widget settings Refresh in widget settings Sep 21, 2020
@bofh80
Copy link
Contributor

bofh80 commented Sep 21, 2020

in:
misc\db_schema.yaml (edit the existing schema for that table)
and:
database\migrations\newfile (copy one existing that fits the purpose and modify ).

That's if i understand correctly. :) I just checked other PR's for the same / similar.

murrant
murrant reviewed Sep 22, 2020
View reviewed changes
app/Http/Controllers/Form/WidgetSettingsController.php Outdated
@@ -45,7 +45,10 @@ public function update(Request $request, $widget_settings)
]);
}

$refresh=(!is_numeric($widget_settings["refresh"]) || (int)$widget_settings["refresh"] < 0 || (int)$widget_settings["refresh"] > 32767 ? 60 : $widget_settings["refresh"]);
Copy link
Member

Choose a reason for hiding this comment

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

This should be done in a validation rule above.

Copy link
Member

Choose a reason for hiding this comment

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

'settings.refresh' => 'int|min:1|max:86400',

Copy link
Contributor Author

@Negatifff Negatifff Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ok, thanks. but I can't understand - I set not int value and after pressing save button I see empty red alert in upper right corner. what I haven't do yet?

murrant
murrant reviewed Sep 22, 2020
View reviewed changes
app/Http/Controllers/Form/WidgetSettingsController.php Outdated
@@ -45,7 +45,10 @@ public function update(Request $request, $widget_settings)
]);
}

$refresh=(!is_numeric($widget_settings["refresh"]) || (int)$widget_settings["refresh"] < 0 || (int)$widget_settings["refresh"] > 32767 ? 60 : $widget_settings["refresh"]);
unset($widget_settings["refresh"]);
Copy link
Member

Choose a reason for hiding this comment

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

might as well unset _token too

@murrant
Copy link
Member

murrant commented Sep 22, 2020 •
edited
Loading

I suggest not using the "refresh" field, just put it in the settings json. Maybe even remove that field.
I also noticed an error in many widgets (the reason you had to set refresh in many widgets)
Inside the getSettings() function they should call parent::getSettings()

@murrant
Copy link
Member

murrant commented Sep 22, 2020

You won't have an empty refresh issue if you call parent::getSettings()

@Negatifff
Copy link
Contributor Author

sorry, but I do not quite understand where to do it

Negatifff and others added 2 commits September 23, 2020 12:23
@Negatifff
fix empty settings for new widgets
a4a151b
@murrant
Set refresh in base
fe7d809 
Update refresh at runtime (doesn't interrupt previous timeout)
@Negatifff
Copy link
Contributor Author

@murrant thanks

@murrant murrant merged commit 173ca83 into librenms:master Oct 2, 2020
@murrant murrant changed the title Refresh in widget settings Add refresh in widget settings Nov 2, 2020
@murrant murrant added the Feature label Nov 2, 2020
@murrant
Copy link
Member

murrant commented Nov 2, 2020

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-69-release-changelog-october-2020/13837/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murrant murrant murrant approved these changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Negatifff @bofh80 @murrant