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

When saving graph settings from the graph page, the graph template id should not be included #1507

Closed
gdsotirov opened this issue Mar 29, 2018 · 15 comments
Labels
enhancement General tag for an enhancement

Comments

@gdsotirov
Copy link
Contributor

As discussed in topic Unsave prevously used Template filter in Graph View, filter settings are saved automatically either to database or session, but there is actually no way to unsave these[1]. So as an effect a previously used filter continues to reappear when loading graph view pages, which is undesirable (at least not always and/or at least for me). I would suggest button "Unsave" or "Forget" in Graph Filters section on Graph view page or eventually allowing the modification of these settings through user's profile page.

[1] Unless clearing the value directly into the database like I've done with 'graph_template_id' setting in settings_user table.

@netniV
Copy link
Member

netniV commented Mar 29, 2018

I'm not sure whether the Clear button actually clears the user settings, though it should. There may need to be a setting against the user for "clear filter settings on login" as an enhancement, though that would be quite a bit of work since I think the filter settings should end up in a table of their own to make it easier to clear them out (as opposed to standard user settings which are editing by the user). That would be quite a number of pages to go through and test.

@gdsotirov
Copy link
Contributor Author

gdsotirov commented Mar 29, 2018

I'm not sure whether the Clear button actually clears the user settings, though it should.

No it doesn't as I reported in the forum.

There may need to be a setting against the user for "clear filter settings on login" as an enhancement

Clearing of filter settings should be possible at any time I think, no just on login...

the filter settings should end up in a table of their own to make it easier to clear them out (as opposed to standard user settings which are editing by the user)

This is confusing for me. The problematic setting in my case (graph_template_id) is already in table settings_user, so its already into the database. Isn't it just passible for this setting to be made modifiable (eventually with option for clearing) from user's profile?

@netniV
Copy link
Member

netniV commented Mar 29, 2018

You are thinking about a singular filter value. I am thinking more longer term and how it should be structured for all the other filter values too.

@gdsotirov
Copy link
Contributor Author

Yes @netniV, my focus is the saved filter value with which I had problem, but I believe my suggestion is also valid in general - if some value could be saved as a user setting, then it should also be possible to unsave/clear it, right? I'm not that familiar with Cacti's source, but as a developer I do not quite see the complexity of this.

@netniV
Copy link
Member

netniV commented Mar 29, 2018

That's because the settings_user table is currently a mixture of user defaults plus filter settings. If the filters were in a table of their own, we can simply empty the values upon login to ensure that the settings were cleared. Instead of hard coding a lot of values, we no longer need to know what they are. All we then need is a user default (in the settings_user) table that says 'Clear filters on login'.

The alternative is that we have to add lots of extra logic for every filter that gets handled to/from the database to only store it if it's wanted to be stored, plus only read it back from the DB under the same circumstances. The above circumvents that by leaving all logic as it is and clears the new table on login.

Please note, this was just my initial thoughts, what actually gets implemented can often change as we have discussions on how to achieve what is desired. Quite often, user opinions like your own sway things in a different direction. Plus, after a day or two, other ideas on how to do it can surface which as a developer I'm sure you've experienced first hand many times :)

@gdsotirov
Copy link
Contributor Author

That's because the settings_user table is currently a mixture of user defaults plus filter settings. If the filters were in a table of their own, we can simply empty the values upon login to ensure that the settings were cleared. Instead of hard coding a lot of values, we no longer need to know what they are.

Its a design decision, but I guess user's filter settings could normally be considered as subset of user's default settings, so I do not necessarily see this as a problem. In this case there could be a column in the table for differentiating subsets of settings, so it's possible to clear or delete all settings by subset.

All we then need is a user default (in the settings_user) table that says 'Clear filters on login'.

I still do not understand why it should be "on login", but I accept it as "initial thoughts". Anyway, shouldn't this be a button labeled just "Clear filters" like "Clear Private date", so it's clickable whenever the user decides? Also I wonder isn't it just enough to display filter settings on user's profile page, so that it's possible to modify these like other user defaults whenever the user decides. As I understand many of the settings from settings_user table are already displayed on the user's profile and could be modified, so I wonder why filter settings are omitted.

The alternative is that we have to add lots of extra logic for every filter that gets handled to/from the database to only store it if it's wanted to be stored, plus only read it back from the DB under the same circumstances.

I do not think all this is necessary as long as there's the simple possibility for clearing any saved filter setting, which is the actual problem.

@cigamit cigamit changed the title Unsave filter settings Allow a user to revert to default settings Apr 4, 2018
cigamit added a commit that referenced this issue Apr 4, 2018
Allow a user to revert to default settings
@cigamit
Copy link
Member

cigamit commented Apr 4, 2018

This is resolved in 1.2 through the profile button or 'Clear User Settings', which will revert all user settings back to the default values.

@cigamit cigamit added the enhancement General tag for an enhancement label Apr 4, 2018
@gdsotirov
Copy link
Contributor Author

@cigamit As I understand 2c51a49 would clear all user settings (i.e. it does DELETE FROM settings_user WHERE user_id = ?), right? However, the intention of this task was only to be able to clear saved filters, so it was titled "Unsave filter settings". I'd just like to clarify this, because clearing all user settings may not be what all users (including me) would always want to do when they just want to clear saved filters.

@netniV
Copy link
Member

netniV commented Apr 4, 2018

@gdsotirov this was partly why I was suggesting separating filter settings into their own table so we are not having to maintain a list of filter names to be cleared. It ensures a clean separation and takes away the risk of a future issue being raised where a filter was not actually added to the list of things to be cleared.

Having the clear all button is still a useful feature though as a user may be having issues and want to reset to defaults.

@gdsotirov
Copy link
Contributor Author

@netniV Yes, I got your point, so why it wasn't done like this? I guess, because it's more work (e.g. all sources reading/writing to settings_user table would have to be modified). I won't argue that having clear all button is a useful feature, but it doesn't actually address my need for clearing just saved filters. So what if I need to clear just saved filters and leave all other settings as they are? I believe it's a valid use case, but there is no such feature now and as I understand there won't be. However, there are about 50 settings in settings_user table in my Cacti installation currently, which I'm not sure I'd like to wipe out when my sole intention is to clear 'graph_template_id' setting for example. I presume I'd have to stick to the manual solution from topic in the forum - i.e. to clear (or delete) the setting with an SQL query.

@netniV
Copy link
Member

netniV commented Apr 4, 2018

The majority of those settings are user defaults which aren’t different from the system defaults. There is an outstanding feature request I submitted to only save those settings which a user actually ticks to override. Again, it isn’t a simple thing to implement and needs consideration. Your request about did highlight the need to clear all user settings to reset a user, so that’s been implemented. The need to clear Just filters requires more thought on the direction to take.

@gdsotirov
Copy link
Contributor Author

Could you please point me to this feature request @netniV, so I could follow it? I just checked and apparently some of my admin user settings are same as system's settings:

SELECT USR.`name` setting_name, USR.`value` usr_value, SYS.`value` sys_value,
       CASE
         WHEN USR.`value` != SYS.`value` THEN 'Yes'
         ELSE 'No'
       END diff
  FROM cacti.settings_user USR,
       cacti.settings      SYS
 WHERE USR.user_id = 1
   AND USR.`name` = SYS.`name`;

+---------------------+-----------+----------------+------+
| setting_name        | usr_value | sys_value      | diff |
+---------------------+-----------+----------------+------+
| axis_font           |           | Ariel          | Yes  |
| axis_size           | 8         | 8              | No   |
| default_datechar    | 0         | 1              | Yes  |
| default_date_format | 4         | 4              | No   |
| legend_font         |           | DejaVuSansMono | Yes  |
| legend_size         | 8         | 8              | No   |
| monitor_grouping    | tree      | default        | Yes  |
| monitor_legend      | on        |                | Yes  |
| monitor_refresh     | 60        | 300            | Yes  |
| monitor_sound       | 0         | 0              | No   |
| monitor_view        | tilesadt  | tilesadt       | No   |
| selected_theme      | modern    | modern         | No   |
| title_font          |           | Ariel          | Yes  |
| title_size          | 8         | 10             | Yes  |
| unit_font           |           | Ariel          | Yes  |
| unit_size           | 8         | 8              | No   |
+---------------------+-----------+----------------+------+
16 rows in set (0,00 sec)

However, these are just 16 out of 50 user settings that I found in 'settings_user' table for the same user. I'm not even sure how, when and why I changed fonts (I remember only tuning monitor plugin appearance recently), so I do not think I need these user settings. Is it save to delete them? I presume Cacti would then fall on system defaults, right?

Anyway, I absolutely agree that there is no need to store user settings unless they override system defaults. I'll be thus looking forward to new Cacti releases that would improve on user settings management.

@cigamit
Copy link
Member

cigamit commented Apr 4, 2018

@gdsotirov, the Graph Template ID should not be saved. Let me check the code.

@cigamit cigamit changed the title Allow a user to revert to default settings When saving graph settings from the graph page, the graph template id should not be included Apr 5, 2018
cigamit added a commit that referenced this issue Apr 5, 2018
This is a slightly different interpretation of the problem as the Graph
Template ID should not have been in the saved filter setting.
@cigamit
Copy link
Member

cigamit commented Apr 5, 2018

The graph template has been removed from the saved settings. Should not have been there in the first place. So, marking this resolved. You can cherry pick the patch if required.

@cigamit cigamit closed this as completed Apr 5, 2018
@gdsotirov
Copy link
Contributor Author

Thanks @cigamit! I'll wait the next release, because If necessary I could easily clear the setting into the database.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement General tag for an enhancement
Projects
None yet
Development

No branches or pull requests

3 participants