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

Fixes #37222 - Show error message if host fails to save #10077

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Manisha15
Copy link

@Manisha15 Manisha15 commented Mar 5, 2024

At present, Error is shown only if host fails to save due to errors caught in foreman or it shows inline errors. But there are cases where host fails to save due to wrong selection of Lifecycle environment or content view where it silently fails to save the host and only show error in production log.

For example, if you specify the wrong lifecycle environment and deploy via foreman_proxy "Content view environment content facets is invalid" appears in production.log but nothing is displayed in GUI and it simply didn't save the host. These error come from different plugins (e.g., here error was from Katello ) and are not stored in base errors therefore, are not displayed.

Screenshot from 2024-03-13 12-51-47

@ianballou
Copy link
Contributor

The error shown there might not make sense to users -- would it make sense to pick some errors that you encounter frequently, catch them, and return a warning with some instructions about how to fix the problem? "Content view environment content facets" is a bit of an internal thing.

@Manisha15
Copy link
Author

The error shown there might not make sense to users -- would it make sense to pick some errors that you encounter frequently, catch them, and return a warning with some instructions about how to fix the problem? "Content view environment content facets" is a bit of an internal thing.

I tried using :messge in Katello content_facet model as well but there I would have to provide the error message which is general enough for all errors. The error I encountered was due to selection of wrong lifecycle environment and there is possibility that the above error is thrown for other cases.

I agree that the error message is not very intuitive but that's the only error message which was caught in the host error messages.

Comment on lines +105 to +108
# add errors from other sources (e.g. Katello) to base errors
errors = @host.errors.full_messages | @host.errors[:base]
@host.errors.delete(:base)
@host.errors.add(:base, errors.join(', '))
Copy link
Member

Choose a reason for hiding this comment

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

The controller should not change the errors. Shouldn't Katello try to explicitly flag the field that triggered the error so it's highlighted as wrong? And if anything, the view should read the base errors if they're there.

We already have some of that processing:

logger.error "Failed to save: #{hash[:object].errors.full_messages.join(', ')}" if hash[:object].respond_to?(:errors)
hash[:error_msg] ||= [hash[:object].errors[:base] + hash[:object].errors[:conflict].map { |e| _("Conflict - %s") % e }].flatten
hash[:error_msg] = [hash[:error_msg]].flatten.to_sentence
if hash[:render]
error(hash[:error_msg], true) unless hash[:error_msg].empty?
render hash[:render]
elsif hash[:redirect]
error(hash[:error_msg]) unless hash[:error_msg].empty?
if hash[:redirect] == :back
redirect_back(fallback_location: send("#{controller_name}_url"))
else
redirect_to hash[:redirect]
end
end
end

Another consideration: the API controller won't have the same validation this way.

@ianballou
Copy link
Contributor

Yeah, a second PR to improve the Katello error I think would round this out nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants