Kiwi Editor - Add understandable flash message #3981
Conversation
DavidKang
force-pushed
the
kiwi-editor-flash-messages
branch
from
73c201e
to
49e4966
Compare
Codecov Report
@@ Coverage Diff @@
## master #3981 +/- ##
==========================================
- Coverage 89.21% 89.17% -0.04%
==========================================
Files 308 309 +1
Lines 18199 18221 +22
==========================================
+ Hits 16236 16249 +13
- Misses 1963 1972 +9
|
DavidKang
force-pushed
the
kiwi-editor-flash-messages
branch
4 times, most recently
from
22ecbd5
to
9a66660
Compare
|
looks much better :) |
DavidKang
added
the
DO NOT MERGE ⚠️
|
DO NOT MERGE, I still improving the error messages |
We added a more understandable error message in the Kiwi Editor and also in the file view when the importation has a failure due to an invalid kiwi file.
We highlighted the repositories and packages that are not valid.
DavidKang
force-pushed
the
kiwi-editor-flash-messages
branch
5 times, most recently
from
3a183d1
to
764b5c4
Compare
DavidKang
removed
the
DO NOT MERGE ⚠️
DavidKang
force-pushed
the
kiwi-editor-flash-messages
branch
from
764b5c4
to
8b537c5
Compare
DavidKang
changed the title
|
@openSUSE/open-build-service, could someone review it, please 😸 |
| @@ -105,6 +106,23 @@ def cleanup_non_project_repositories! | |||
| @image.repositories.delete_all | |||
| params[:kiwi_image].delete(:repositories_attributes) | |||
| end | |||
|
|
|||
| def image_error_messages(title, image, packages) | |||
There was a problem hiding this comment.
I think this should go in a model not the controller?
| if record.errors.present? | ||
| message["#{key.capitalize}: #{record.name}"] ||= [] | ||
| message["#{key.capitalize}: #{record.name}"] << record.errors.messages.map { |_key, value| value } | ||
| message["#{key.capitalize}: #{record.name}"].flatten! |
There was a problem hiding this comment.
Why is this method necessary? Can we not just use the errors object/hash that ActiveRecord gives us?
There was a problem hiding this comment.
Because the error hash gives us 'Repository[0]...` and I'm trying to do this more understandable grouping all the errors related to each element.
There was a problem hiding this comment.
I guess what I dont like so much about this method is that it fixes a more general problem with active record validations on nested fields so IMO the ideal would be to somehow override ActiveRecord's validation and fix the problem there.. However that might not be so easy to do.. There is some discusion in this PR about how to do that but the only solution offered is to just convert the keys to what you want like David is doing here:
Maybe can you rename this method to something like parsed_errors and then include a comment explaining why we need to convert the original error keys from ActiveRecord?
| records.each do |record| | ||
| if record.errors.present? | ||
| message["#{key.capitalize}: #{record.name}"] ||= [] | ||
| message["#{key.capitalize}: #{record.name}"] << record.errors.messages.map { |_key, value| value } |
There was a problem hiding this comment.
Can do:
record.errors.messages.values| @@ -20,7 +20,8 @@ def import_from_package | |||
| if package.kiwi_image.blank? || package.kiwi_image.destroyed? | |||
| package.kiwi_image = ::Kiwi::Image.build_from_xml(package.source_file(kiwi_file), package.kiwi_file_md5) | |||
| unless package.save | |||
| errors = ["Kiwi File '#{kiwi_file}' has errors:", package.kiwi_image.errors.full_messages].join('<br />') | |||
| errors = image_error_messages("Kiwi File '#{kiwi_file}' has errors:", | |||
| package.kiwi_image, [package.kiwi_image.package_groups.map(&:packages)].flatten.compact) | |||
| redirect_to package_view_file_path(project: package.project, package: package, filename: kiwi_file), error: errors | |||
There was a problem hiding this comment.
Why not render the show page like its done in #update instead of redirecting?
render action: :showThere was a problem hiding this comment.
Because we are redirecting to package_view_file and this needs to set other instances variables.
DavidKang
force-pushed
the
kiwi-editor-flash-messages
branch
4 times, most recently
from
6e5231d
to
88bdff2
Compare
|
@evanrolfe, could you review it again, please 😸 |
We added test for flash_content helper and also included Haml::Helpers.
DavidKang
force-pushed
the
kiwi-editor-flash-messages
branch
from
88bdff2
to
3c23ca4
Compare
We added a more understandable error message in the Kiwi Editor and also in the file view when the importation has a failure due to an invalid kiwi file. We also highlight the repository or package that has an error.
Before
After