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

Add save all transactions functionality #1214

Merged
merged 7 commits into from
May 8, 2024

Conversation

irakliy01
Copy link
Contributor

@irakliy01 irakliy01 commented Apr 21, 2024

πŸ“· Screenshots

image

πŸ“„ Context

I usually test my apps on an emulator that isn't connected to Google services. This means I can't directly share transactions with others, as I don't have suitable apps for that purpose. Instead, it's much easier for me to save transactions locally and then share the file from my computer using Device Explorer.
Chucker only supported sharing transaction details as text or HAR but lacked direct file saving capabilities (you could save only a one request/response body).
This PR adds a feature to save all transactions on the local storage. New save buttons are located within share menu separated from share buttons group by divider.

πŸ“ Changes

  • Introduced "Save as Text" and "Save as HAR" buttons to the share menu, grouped together and visually separated from existing share options.
  • Implemented save-to-file logic, allowing users to save transactions data locally in either plain text or HAR format.
  • Developed a FileSaver object to streamline the file-saving process within the library.
  • Included a JUnit test to ensure the FileSaver.saveFile method writes files correctly to the given URI and with the expected content.

πŸ› οΈ How to test

Click on the share button in the app bar and then click on save as text / save as .har file. Check created files in a directory you specified.

* Impacted screen: Main transaction list
* New UI elements: Save buttons (grouped, separated from share buttons with the divider)
* Save options: Text file, HAR file
* Internal: Create FileSaver object for file writing
* Add kotlinx-coroutines-test dependency
* Test verifies correct file content is written using the provided URI
@irakliy01 irakliy01 requested a review from a team as a code owner April 21, 2024 23:04
Add a line to the unreleased block about save all transactions to the file
Copy link
Member

@cortinico cortinico 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 sending this over @irakliy01
I believe we can include this πŸ‘

assertThat(file.length()).isEqualTo(TEST_FILE_CONTENT.length)
assertThat(file.readText()).isEqualTo(TEST_FILE_CONTENT)

file.delete()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you delete the file here? YOu should instead use a JUnit temp folder if you're afraid this will pollute other test runs

Comment on lines 16 to 17
): Boolean {
return withContext(Dispatchers.IO) {
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
): Boolean {
return withContext(Dispatchers.IO) {
): Boolean = withContext(Dispatchers.IO) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

Comment on lines 430 to 431
): Source {
return when (type) {
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
): Source {
return when (type) {
): Source = when (type) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

HAR -> saveHarToFile.launch(EXPORT_HAR_FILE_NAME)
}
},
onNegativeClick = null,
Copy link
Member

Choose a reason for hiding this comment

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

this is unneceesary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no action on dismiss (old actions also set this listener as null)

@irakliy01
Copy link
Contributor Author

@cortinico updated PR

@cortinico cortinico enabled auto-merge (squash) May 8, 2024 09:59
@cortinico cortinico merged commit 9a24876 into ChuckerTeam:main May 8, 2024
7 checks passed
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.

None yet

2 participants