Jupyterlab extension for logging into a Marble node#1
Jupyterlab extension for logging into a Marble node#1alexandercyu wants to merge 32 commits intomainfrom
Conversation
Add Get Marble Session button to notebook toolbar
- Add code cell with pre-filled code - run the cell (currently doens't seem to be running but the return on the function runCells implies it is running)
Style input fields for node ID, username, and password
- convert python input and password fields to jupyterlab widgets - add submit button - display node list as a dropdown list
- change input fields to jupyterlab widgets -style each input widget and align vertically - make label for each input widget visible - add login status widgets (Note: Not displaying properly) - remove icon for successful login - remove decorators for output widget capture - remove metadata form example
- Remove unneeded getpass import - add text colour to input fields - remove border (border radius not a valid style key)
- change input field and button colours to use hex code from design - add error message for when no choice is made for node name - add conditions to display proper error message according to user 's dropdown choice
Remove Marble design css (moved styling to ipywidgets and Jupyter widgets)
Nicer indentation for json that adds button to notebook toolbar
Remove print statements
mishaschwartz
left a comment
mishaschwartz
left a comment
There was a problem hiding this comment.
All of the relevant code should be moved up one directory level: everything under the top-level jupyterlab_marble_extensions folder should be at the top level of the repo.
.idea/workspace.xml
Outdated
There was a problem hiding this comment.
".idea/" added in gitignore under Pycharm heading.
| # Changes here will be overwritten by Copier; NEVER EDIT MANUALLY | ||
| _commit: v4.3.8 | ||
| _src_path: https://github.com/jupyterlab/extension-template | ||
| author_email: test@test.com |
There was a problem hiding this comment.
add a real email here or leave it blank
There was a problem hiding this comment.
Removed the email
There was a problem hiding this comment.
Do we want to use binder with this? If so we should set up a github token for this action
There was a problem hiding this comment.
I don't think binder will be needed. It was one of the steps in the setup tutorial.
| # Changelog | ||
|
|
||
| <!-- <START NEW CHANGELOG ENTRY> --> | ||
|
|
There was a problem hiding this comment.
Add an initial entry here describing your changes
There was a problem hiding this comment.
Added entry for not automatically installing python packages and an entry for checking for empty credentials before sending request to server.
| */ | ||
|
|
||
| describe('jupyterlab-marble-extension', () => { | ||
| it('should be tested', () => { |
There was a problem hiding this comment.
What are some real tests that we can add here?
There was a problem hiding this comment.
Maybe test if a cell is added to the notebook when the function is run?
| '\n payload["credentials"]["password"] = change["new"];' + | ||
| '\n ' + | ||
| '\n def submit(arg1):' + | ||
| '\n userNode = "" ' + |
There was a problem hiding this comment.
no need to initialize variables here to the empty string
There was a problem hiding this comment.
Removed variables that are initialized to empty string.
| '\n userNode = payload["selectedNode"]' + | ||
| '\n url = MarbleClient()[userNode].url + "/magpie/signin" ' + | ||
| '\n response = requests.post(url, headers={"Content-Type": "application/json"}, json=payload["credentials"])' + | ||
| '\n if("200" in str(response)):' + |
There was a problem hiding this comment.
This is not a safe way to check for a success. The repr of a response object is not stable and could be anything that may or may not contain the status message. Either compare the status_code or ok attributes directly or call raise_for_status()
There was a problem hiding this comment.
Changed success check to checking the response.json().
| '\n submitButton = ipywidgets.Button(description="Submit", disabled=False,button_style="", tooltip="Submit", icon=""' + | ||
| ', style={"font_family":"Helvetica Neue","font_size":"16px", "button_color":"#304FFE", "text_color":"white"} )' + | ||
| '\n ' + | ||
| '\n loginSuccessLabelOutputWidget = ipywidgets.Output()' + |
There was a problem hiding this comment.
You can just have one output widget and write the error or success message to that single output.
There was a problem hiding this comment.
Reduced output widgets to just one and put different output messages to it.
The success and error messages come from the response while the choose another node message is custom.
| '\n with passwordOutput:' + | ||
| '\n payload["credentials"]["password"] = change["new"];' + | ||
| '\n ' + | ||
| '\n def submit(arg1):' + |
There was a problem hiding this comment.
arg1 is not used. To signal that a variable is unused in python, add an underscore before it like _arg
There was a problem hiding this comment.
Added underscore to the variable name
| '\n loginSuccessLabelWidget = ipywidgets.HBox([ipywidgets.Label(value="Logged into " + userNode + " successfully.", style={"text_color":"green", "font_size":"16px"})]) ' + | ||
| '\n loginFailedLabelWidget = ipywidgets.HBox([ipywidgets.Label(value="Error Logging In", style={"text_color":"red", "font_size":"16px"})])' + | ||
| '\n ' + | ||
| '\n if("selectedNode" in payload and payload["selectedNode"] != "Node ID"):' + |
There was a problem hiding this comment.
Why don't we also add in a check that username and password are non-empty before we submit the request so that we can tell the users that those values are required as well.
There was a problem hiding this comment.
Added checks for username and password to check if they're non-empty.
Added error message for empty username or password.
Error messages for username, password, node name should clear correctly.
Error messages will also stack if a node name is invalid and username or password, or both, are empty.
Added Pycharm specific ignore
- output widget messages no longer stack up except in the case of an invalid node name and empty user credentials - changed request.post() to session.post() - successful or erroneous login feedback messages is now the one from the server response - removed variables that are initialized as empty strings - changed success check condition to use response.json() - removed unneeded output widgets - added check for empty credentials
Add changelog items
Changed command name to marble_login
Added test to see if a code cell is added to the top of the notebook (index zero)
Removed command call
Removed email
Removed email
|
@alexandercyu you haven't pushed any changes since my last review. If you're working on another branch you need to merge the changes here or else they won't show up on this PR. |
Add blank line to satisfy prettier linting error
Added tests (tests in progress)
Comment out test for now (test isn't working)
Updated example
Merged the changes from updated-example into this branch. |
Added new entries for pycharm, yarn, node_modules, and testing specific directories
hatchling
Outdated
There was a problem hiding this comment.
I don't know. It was part of the extension template from the tutorial: https://jupyterlab.readthedocs.io/en/latest/extension/extension_tutorial.html
This line copies the template from the url: copier copy --trust https://github.com/jupyterlab/extension-template .
src/index.ts
Outdated
| '\nexcept ImportError as exc:' + | ||
| '\n raise Exception("The marble_client package is required to run this cell. Please install it and run this code again.") from exc' + | ||
| '\n ' + | ||
| '\nfrom marble_client import MarbleClient' + |
There was a problem hiding this comment.
This is already imported above
There was a problem hiding this comment.
Removed duplicated import.
| '\n client_session = client.this_session()' + | ||
| '\nexcept JupyterEnvironmentError:' + | ||
| '\n ' + | ||
| '\n session = requests.Session()' + |
There was a problem hiding this comment.
This variable should be the same as in the try block
There was a problem hiding this comment.
Changed 'client_session = client.this_session()' in the try block to 'session = client.this_session()'.
src/index.ts
Outdated
| '\n password_widget = ipywidgets.Password(style=input_field_style)' + | ||
| '\n password_box_widget = ipywidgets.VBox([password_label_widget, password_widget])' + | ||
| '\n ' + | ||
| '\n submit_button = ipywidgets.Button(description="Submit", button_style="", tooltip="Submit", icon=""' + |
There was a problem hiding this comment.
button_style and icon not needed
There was a problem hiding this comment.
Removed button_style and icon from the button definition.
| '\n input_field_style = {"description_width":"initial"}' + | ||
| '\n login_success_style = {"font_family":"Helvetica Neue","font_size":"16px", "text_color":"green"}' + | ||
| '\n error_style = {"font_family":"Helvetica Neue","font_size":"16px", "text_color":"red"}' + | ||
| '\n choose_another_node_style = {"font_family":"Helvetica Neue","font_size":"16px", "text_color":"#304FFE"}' + |
There was a problem hiding this comment.
Think about making the style into a variable to be consistent
There was a problem hiding this comment.
Made the submit button style into a variable.
src/index.ts
Outdated
| '\n ui_message_output_widget = ipywidgets.Output()' + | ||
| '\n ui_message_label_widget = ipywidgets.Label(value="", style=ui_label_style)' + | ||
| '\n ui_message_display_box_widget = ipywidgets.HBox([ui_message_label_widget])' + | ||
| '\n credential_error_output_widget = ipywidgets.Output()' + |
There was a problem hiding this comment.
I'd recommend just having one output and you can choose what to display in there as needed
There was a problem hiding this comment.
Reduced to one output widget.
Removed HBox widgets for outputs.
src/index.ts
Outdated
| '\n response = session.post(url, headers={"Content-Type": "application/json"}, json=payload["credentials"])' + | ||
| '\n response_json = response.json()' + | ||
| '\n ' + | ||
| '\n if(response_json["code"] == 200):' + |
There was a problem hiding this comment.
Now checks response.status_code for 200 response.
Also gets details of response from response.reason if there is no response.json().
- Removed Hbox widgets for displaying output messages, just use label widgets instead. - reduce output widgets to one - check for server response using response.status_code instead - if status_code is 200, then check for magpie specific json and get magpie specific response message. Otherwise use standard response message
Deleted file
2 participants
Adds a button to the Jupyter Lab toolbar that will add a cell to the top of a Jupyter notebook with prepopulated code. The code will create and display a login interface that allows the user to choose a node to log in to and enter their login credentials.