Skip to content

Conversation

@wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Aug 2, 2021

Important

The PR TACC/Core-CMS-Resources#67 is dependent on this PR.

To Do

Footnotes

* Rename to be consistent with upcoming system_specs and possible system_....
† Either (a) do not use GetElementById or (b) limit plugin to one instance. Try not to over-engineer the JavaScript, because #295 will delete it.

Overview

Implement minimal SysMon plugin using polished CSS and JavaScript.

Issues

Changes

  • Create plugin from snippet.

Screenshots

Core

Toggle visibility

Form

Sysmon Plugin - Admin Form

Render

Sysmon Plugin - Core Styles

Structure

System Plugin - in Structure

Frontera

Toggle visibility

Render

Sysmon Plugin - Frontera Home - Render

Structure

Sysmon Plugin - Frontera Home - Structure

Testing

Last recorded deploy to Frontera (dev) was Build #383 (Aug 2, 2021 1:58:12 PM).

Important: If you tested this plugin before it was renamed from "Sysmon" to "System Monitor", then you must follow these cleanup steps to prevent a CMS server crash:

Steps for Plugin Rename
  1. Checkout commit or branch that does not crash server.
    • If you have only been testing this plugin, then a36e978 should work.
    • If you have been testing several plugins, then site/frontera/main might work.
  2. Delete "Sysmon" plugin instances:
    python3 manage.py cms uninstall plugins TaccsiteSysmonPlugin
    
  3. Checkout task/GH-89 branch at latest commit.
  4. Perform database migrations for new "System Monitor" plugin:
    python3 manage.py migrate
    
  5. Re-add deleted plugin instances (as necessary for testing) using new "System Monitor" plugin.
  6. Rebuild assets and collect static files:
    npm run build && docker exec -it core_cms python manage.py collectstatic --no-input
    
  1. If no environment, then prepare to test CMS changes.
  2. In CMS container, run python3 manage.py migrate.
  3. On CMS website, logged in as admin, add "TACC Site" > "System Monitor" plugin.
  4. Reload page (to trigger client-side* data retrieval).
  5. Ensure the following:
    • styles match system monitor on [Old Homepage][old-home] / [Phase 0 Homepage][new-home]
      • the text may be larger; this is acceptable†
    • data shown is for system chosen
    • plugin description (next to name) in Structure view is the system chosen
  6. Add "Generic" > "Text" plugin.
  7. In Text plugin, in "CMS Plugins" dropdown, confirm "TACC Site" > "System Monitor" plugin is not available.
  8. Add another "TACC Site" > "System Monitor" plugin, and choose "Stampede2".
  9. Ensure Stampede2 monitor—
    • if on local server—has "Warning" state
    • if on remote server—is populated with live data
Footnotes

* I hope to implement server-side retrieval later via #295.
† Different projects and templates have different root font size. (Values have been standardized, but implementation has not caught up to standards.)

Notes

This is a minimal version of a functionally better plugin, bitbucket:rochaa/tacc-sysmon bitbucket.org:tacc-system-monitor. @tacc-wbomar has deadline to finish #88 (which requires #89) this week. So #295 has been created to enhance this plugin later.

@wesleyboar wesleyboar linked an issue Aug 2, 2021 that may be closed by this pull request
@wesleyboar wesleyboar changed the title GH-89: Initial SysMon plugin (functional) GH-89: (Minimal) System Monitor Plugin (a.k.a. SysMon) Aug 2, 2021
@wesleyboar wesleyboar added the priority ━ Medium priority label Aug 8, 2021
Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Added notes for other reviewers.

@wesleyboar wesleyboar requested a review from iamthatian August 9, 2021 17:02
Copy link
Contributor

@iamthatian iamthatian left a comment

Choose a reason for hiding this comment

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

I've approved! However, I also included some minor questions/comments.

Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

@duckonomy, thanks. I've responded.

Also, what you think of renaming this plugin (to mirror system_specs plugin name)?

Screen Shot 2021-08-11 at 16 02 46

@iamthatian
Copy link
Contributor

iamthatian commented Aug 11, 2021

@duckonomy, thanks. I've responded.

Also, what you think of renaming this plugin (to mirror system_specs plugin name)?

Screen Shot 2021-08-11 at 16 02 46

@tacc-wbomar That sounds good!

@iamthatian
Copy link
Contributor

@tacc-wbomar For 2,3 I think those are valid reasons 👍🏼

@wesleyboar wesleyboar requested a review from iamthatian August 12, 2021 19:45
@wesleyboar
Copy link
Member Author

@duckonomy I have re-requested review merely to remove approval to help remind me that this PR is not complete. There is nothing new to review, yet.

@wesleyboar wesleyboar added priority ▲ High priority and removed priority ━ Medium priority labels Aug 13, 2021
@wesleyboar wesleyboar mentioned this pull request Aug 13, 2021
@wesleyboar
Copy link
Member Author

@duckonomy I completed the To Do items. Code was changed only for:

  • removal of errant import (a36e978)
  • plugin rename (0d9491f)
    • To test, follow "Steps for Plugin Rename" in "Testing" section of description.
  • addition of missing functions (871676e)
    • I found this bug during my re-test.

Please review.

Copy link
Contributor

@iamthatian iamthatian left a comment

Choose a reason for hiding this comment

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

The plugin works great!

There was one configuration value that was causing an error on docker-compose up.
Other than that, I've noticed this time that the plugin didn't work when having multiple instances (of Frontera).
Is this intended behavior?

Comment on lines 244 to 247
'taccsite_cms.contrib.taccsite_static_article_list',
'taccsite_cms.contrib.taccsite_static_article_preview',
'taccsite_cms.contrib.taccsite_blockquote',
'taccsite_cms.contrib.taccsite_offset',
Copy link
Contributor

Choose a reason for hiding this comment

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

These cause an error on initialization (I had to comment them to make it run properly).

Copy link
Member Author

@wesleyboar wesleyboar Aug 16, 2021

Choose a reason for hiding this comment

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

That is a problem. [All] of those should not be [in this branch]. This problem suggests that more code may be present that should not be. I will investigate. Thank you.

Details

There are so many dependent branches for Frontera System Specs page and Frontera Homepage plugins that I am trying to author independently but test together 🤕 and I crossed the streams.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed those lines (via 5ed2892). No related erroneous code was found, but I found and resolved* a different unexpected change in the submodule diff.

Footnotes

* I resolved unexpected submod diff with a merge of main, a2807a2 (after a submod pointer update: #315).

@wesleyboar
Copy link
Member Author

wesleyboar commented Aug 16, 2021

Good catch: the multiple instances bug.

I have added a task to either support multiple instances or not.

Details

The problem is caused by the JavaScript I borrowed and cleaned up still using document.getElementById. There really should be only one instance of this plugin on a page. I need to either (with Python) prevent multiple instances or (with JavaScript) allow multiple instances.

Use `element.querySelector('[data-id=""]')`.
Do not use `document.getElementById`.
Use JavaScript module and class.
Do not use window variables and functions.
Why?
1. The Bootstrap "success" pill color is used.
    - See `badge-success` in HTML but not CSS.
2. The custom warning color was recently removed.
    - #312
3. The Portal success, warn, etc colors are not standardized.
    - They will be in FP-1145, but CMS's could differ.
@wesleyboar
Copy link
Member Author

wesleyboar commented Aug 17, 2021

@duckonomy, fixes:

  • Multiple instances are now functional.

    For just the diff relevant to this…

    I suggest reviewing first 30c78a6, then 7220bc1 (instead of "changes since last review").

  • Also, the warning state is now testable (see new "Testing" step 8 & 9).

    For just the diff relevant to this…

    I suggest reviewing 333918d (instead of "changes since last review").

@wesleyboar wesleyboar requested a review from iamthatian August 17, 2021 18:48
Copy link
Contributor

@iamthatian iamthatian left a comment

Choose a reason for hiding this comment

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

LGTM!

@wesleyboar wesleyboar merged commit 5b97b8f into main Aug 17, 2021
@wesleyboar wesleyboar deleted the task/GH-89 branch August 17, 2021 22:25
@wesleyboar wesleyboar mentioned this pull request Aug 24, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority ▲ High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System Monitor - Styles & Plugin

3 participants