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

95 bugfix/form not hyperlinked #96

Merged
merged 9 commits into from
Oct 26, 2023
Merged

Conversation

louisevelayo
Copy link
Collaborator

Fixes #95

Description

Include a link to a feedback form in all messages sent from Node Monitor.

Changes made

  • Load form URL from .env and pass as an argument to functions in messages.py.
  • Update test_bot_telegram and test_bot_slack to use message templates when sending live messages during testing.

Testing

  • Ran entire testing suite and all checks passed.

Additional comments

n/a

@louisevelayo louisevelayo linked an issue Oct 23, 2023 that may be closed by this pull request
Copy link
Contributor

@mourginakis mourginakis left a comment

Choose a reason for hiding this comment

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

Thanks for adding fake node data to test_bot_telegram.py and test_bot_slack.py

node_monitor/node_monitor_helpers/messages.py Outdated Show resolved Hide resolved
node_monitor/load_config.py Outdated Show resolved Hide resolved
@mourginakis
Copy link
Contributor

mourginakis commented Oct 24, 2023

I think it may be better if we use unittest.mock.patch to change the URL, or just replace any URLs in the string with a regex. Thoughts?

Edit: I just did this with unittest.mock.patch. I like it a lot more like this, it keeps the messages API simple.

@mourginakis mourginakis self-requested a review October 24, 2023 19:20
Comment on lines 31 to 49
## Create a fake node model
fakenode = ic_api.Node(
dc_id = 'fake_dc_id',
dc_name = 'fake_dc_name',
node_id = 'fake_node_id',
node_operator_id = 'fake_node_operator_id',
node_provider_id = 'fake_node_provider_id',
node_provider_name = 'fake_node_provider_name',
owner = 'fake_owner',
region = 'fake_region',
status = 'DOWN',
subnet_id = 'fake_subnet_id',
)
fakelabel = {'fake_node_id': 'fake_label'}

with patch.object(c, 'FEEDBACK_FORM_URL', 'https://url-has-been-redacted.ninja'):
subject1, message1 = messages.nodes_down_message([fakenode], fakelabel)
subject2, message2 = messages.nodes_status_message([fakenode], fakelabel)

Copy link
Contributor

Choose a reason for hiding this comment

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

We're repeating this code 3 times.
I suggest one of:

  1. We test sending a short simple string in both test_bot_slack.py and test_bot_telegram.py like before, and use only the public inbox to make sure the messages look like how we want them. Keep the codebase a bit smaller.
  2. We move this logic outside of these test functions, compute once and store it in conftest.py.
  3. Leave as is.

I'm kind of in favor of option 1. Thoughts?

@mourginakis mourginakis self-requested a review October 25, 2023 19:43
Copy link
Contributor

@mourginakis mourginakis left a comment

Choose a reason for hiding this comment

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

Can we get rid of any of these unused imports? Specifically the ones in test_bot_slack and test_bot_telegram?

@mourginakis mourginakis self-requested a review October 26, 2023 17:12
@mourginakis mourginakis merged commit dce7f9b into main Oct 26, 2023
1 check passed
@mourginakis mourginakis deleted the 95-bugfix/form-not-hyperlinked branch October 26, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bugfix/form-not-hyperlinked
2 participants