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

WIP: #190 JSON export #236

Merged
merged 8 commits into from
Mar 22, 2021
Merged

WIP: #190 JSON export #236

merged 8 commits into from
Mar 22, 2021

Conversation

GneyHabub
Copy link
Contributor

@GneyHabub GneyHabub commented Mar 10, 2021

Now there are 2 buttons over each Content field of the Messages table.

image

If you hover over any of them there will be a popup hint which tells the user what this button does:

image
image

The first button will copy the Content to the clipboard and the second will trigger a file download (.json if the content is an object or JSON.parse doesn't throw an exception; .txt otherwise.)

@workshur
Copy link
Member

  1. Please add description
  2. Add screenshot

Copy link
Member

@workshur workshur left a comment

Choose a reason for hiding this comment

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

  1. We need to move buttons to JSONViewer component.
  2. Use standard bulma buttons

kafka-ui-react-app/src/components/App.scss Outdated Show resolved Hide resolved
Abstract dynamic button to a separate component;
Move buttons to the JSONViewer;
Move data-saving to a hook;
@GneyHabub
Copy link
Contributor Author

I have no idea how to test the file download yet. Still working on it.

@GneyHabub
Copy link
Contributor Author

Okay, after some research I've got an impression that the file-downloading part of the functionality is pretty much untestable. I didn't find a way to trigger the file download inside of the testing environment.
It is possible to dig into the specific implementation and test it, but this seems wrong to me. In this way, we treat the component as a white box, exposing all of its guts, tying our tests to the specific implementation. Looks like a violation of a bunch of principles and good practises.
@workshur @whotake what do you think?

@GneyHabub GneyHabub requested a review from workshur March 16, 2021 10:47
@workshur workshur requested a review from whotake March 16, 2021 11:58
@workshur workshur linked an issue Mar 17, 2021 that may be closed by this pull request
@workshur workshur marked this pull request as ready for review March 17, 2021 09:32
@whotake
Copy link
Contributor

whotake commented Mar 17, 2021

@GneyHabub Does it throw any errors when are you trying to test it?

I think we can split view and download logic and test them separately:

  1. Test view that it calls saveFile with proper arguments
  2. Test saveFile itself. And there a few options on how to do it:
    2.1 Split function into modules and test important logic. We care about file name and content more than pretty much standard downloading function.
    2.2 Mock document and check setAttribute agruments
    2.3 Combine two methods above

@GneyHabub GneyHabub requested a review from whotake March 19, 2021 09:25
@sonarcloud
Copy link

sonarcloud bot commented Mar 22, 2021

@sonarcloud
Copy link

sonarcloud bot commented Mar 22, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@GneyHabub GneyHabub merged commit 7c86adb into master Mar 22, 2021
@GneyHabub GneyHabub deleted the feature/#190-json-export branch March 22, 2021 15:36
javalover123 pushed a commit to javalover123/kafka-ui that referenced this pull request Dec 7, 2022
* Implement functionality for copying and downloading data

* Test
Donutellko pushed a commit to Donutellko/kafka-ui that referenced this pull request Jul 12, 2024
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.

Topic message : json export feature
3 participants