Skip to content

Conversation

sukhwinder33445
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 commented Jan 30, 2025

@sukhwinder33445 sukhwinder33445 added the bug Something isn't working label Jan 30, 2025
@sukhwinder33445 sukhwinder33445 self-assigned this Jan 30, 2025
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jan 30, 2025
@ncosta-ic
Copy link
Contributor

@sukhwinder33445 I feel like it looks kinda weird with the current implementation, as it still shows the name input, even though there's no submit button or the like.

What if you moved the result check to the top of the assemble method? I think this would correlate with our philosophy of having early breakouts if required data is not provided to a method.

It currently looks like this:
image

And adding the check and early return to the top of the method would result in this representation:
image

The displayed message is also missing a dot (.) at the end of the second sentence.

Keep in mind that I only wondered about the reasoning for this exact implementation, this isn't really a suggestion. But if you do feel like it would improve the UI, feel free to change it. Will do a proper review after your comment on this topic :)

@nilmerg
Copy link
Member

nilmerg commented Feb 25, 2025

Our actual philosophy on this topic is not allowing access to a form at all. i.e. to disable the button that leads to it and adding a title to explain why.

@sukhwinder33445 sukhwinder33445 force-pushed the fix/channel-config-stacktrace branch from 6152779 to c7e057e Compare February 25, 2025 10:28
@nilmerg
Copy link
Member

nilmerg commented Feb 25, 2025

…or to hide the button but still explain why if there is no contextual hint.

@sukhwinder33445 sukhwinder33445 force-pushed the fix/channel-config-stacktrace branch from c7e057e to 60c1a94 Compare June 23, 2025 14:23
@sukhwinder33445 sukhwinder33445 force-pushed the fix/channel-config-stacktrace branch 2 times, most recently from 8f0da55 to a69c7a4 Compare July 23, 2025 14:08
@sukhwinder33445 sukhwinder33445 requested review from nilmerg and removed request for ncosta-ic July 23, 2025 14:11
->execute();

if (! $query->hasResult()) {
throw new IcingaException('No channel types available. Make sure Icinga Notifications is running.');
Copy link
Member

Choose a reason for hiding this comment

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

Throw a ConfigurationError instead.

@@ -112,7 +120,7 @@ public function addAction()
$form->getValue('name')
)
);
$this->redirectNow(Links::channels());
$this->redirectNow('__CLOSE__');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->redirectNow('__CLOSE__');
$this->switchToSingleColumnLayout();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this change to #301.

@sukhwinder33445 sukhwinder33445 force-pushed the fix/channel-config-stacktrace branch 3 times, most recently from 10d36d1 to 0e3e60d Compare July 25, 2025 13:44
@sukhwinder33445 sukhwinder33445 requested a review from nilmerg July 25, 2025 13:45
@sukhwinder33445 sukhwinder33445 force-pushed the fix/channel-config-stacktrace branch from 0e3e60d to 9238dc4 Compare July 25, 2025 14:06
@sukhwinder33445 sukhwinder33445 force-pushed the fix/channel-config-stacktrace branch from 9238dc4 to 9fc7837 Compare September 12, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module Channel Configuration: No Available Channels results in Stack Trace for Add Channel
3 participants