[DDW-248] Refactor compress/download logs handling #995

thedanheller · 2018-06-26T20:03:52Z

This PR refactors compress/download logs handling.

Todo:

Download logs should always pack fresh logs into logs.zip file
Bug report should always pack fresh logs into logs.zip file and submit it to the Report server.
Create logs.zip files with timestamp [1].

[1] Format based in this answer.

Screenshots:

Normal state

Submitting state (Close button is disabled, 50% opacity)

Review Checklist:

Basics

Code Quality

Important parts of the code are properly documented and commented
Code is properly typed with flow
React components are split-up enough to avoid unnecessary re-rendering
Any code that only works in Electron is neatly separated from components

Testing

New feature / change is covered by acceptance tests
All existing acceptance tests are still up-to-date
New feature / change is covered by Daedalus Testing scenario
All existing Daedalus Testing scenarios are still up-to-date

After Review:

Merge PR
Delete source branch
Move ticket to done on the Youtrack board

nikolaglumac

Great work @daniloprates 👍

I see only a couple of issues:

compressed log files name should contain the UTC timestamp instead of the local time zone time (please see how we do it here: https://github.com/input-output-hk/daedalus/blob/develop/source/main/utils/setupLogging.js#L19)
support-request dialog generates the logs in the moment of opening of the dialog instead of in the moment user clicks on "Submit" (this is bad as it can happen that the user leaves the dialog open for a long time and then submitted logs will be missing the part of the data)
even if the user keeps the "attach-logs" switch off the support request will still attach his logs which should not happen:
when you open the support-dialog the submit button shows with the spinner (as if something is happening) - probably due to the fact you are compressing the logs on dialog mount (which should not be the case):

The last of the issues on the list points me into direction of thinking that this issue actually hasn't been resolved: https://iohk.myjetbrains.com/youtrack/oauth?state=%2Fyoutrack%2Fissue%2FDDW-246

Thanks for fixing this!

nikolaglumac · 2018-07-04T10:52:44Z

source/common/fileName.js

@@ -0,0 +1,7 @@
+import moment from 'moment';


@daniloprates please add // @flow to the top of this file.

nikolaglumac · 2018-07-04T10:53:39Z

source/common/fileName.js

+  `${prefix}-${moment().format('YYYY-MM-DDThhmmss')}.${filetype}`;
+
+export const getFilenameWithTimestamp = (prefix = 'logs', filetype = 'zip') => fileName =>
+  fileName.match(RegExp(`(${prefix}-)([0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{6})(.${filetype})`));


@daniloprates this regexp will have to be updated once you change the filenames to contain timestamp in UTC.

nikolaglumac · 2018-07-04T10:54:24Z

source/main/utils/setupLogging.js

@@ -19,4 +21,19 @@ export const setupLogging = () => {
    const formattedDate = moment.utc(msg.date).format('YYYY-MM-DDTHH:mm:ss.0SSS');
    return `[${formattedDate}Z] [${msg.level}] ${msg.data}`;
  };
+
+  // Removes existing logs
+  fs.readdir(appLogsFolderPath, (err, files) => {


@daniloprates this part works great 🎉

nikolaglumac · 2018-07-04T10:55:49Z

source/renderer/app/components/profile/bug-report/BugReportDialog.js

  };

  componentWillMount() {
-    this.props.onGetLogs();
-    this.props.onDeleteCompressedLogs();
+    this.props.onGetFreshLogs();


@daniloprates we need to make sure this call only fetches the log files list and doesn't compress them!

Even when the user submits it?

nikolaglumac · 2018-07-04T10:56:39Z

source/renderer/app/components/profile/bug-report/BugReportDialog.js

-          // regular submit
-          this.props.onSubmit(data);
-        }
+        this.props.onSubmit(data);


@daniloprates we shouldn't send the log files if the user has set the "attach-logs" switch to off state.

nikolaglumac · 2018-07-04T10:57:47Z

source/renderer/app/containers/profile/dialogs/BugReportDialogContainer.js

@@ -61,8 +65,8 @@ export default class BugReportDialogContainer extends Component<InjectedProps> {
        onGetLogs={() => {
          getLogs.trigger();
        }}
-        onCompressLogs={(logs) => {
-          compressLogs.trigger({ logs });
+        onGetFreshLogs={(logs) => {


@daniloprates what is the difference between onGetLogs and onGetFreshLogs?
If the later one actually compresses the logs then I think the name should be changed...

Renamed it to avoid confusion

nikolaglumac · 2018-07-04T10:58:17Z

source/renderer/app/stores/ProfileStore.js

  _compressLogs = action(({ logs }) => {
    this.isCompressing = true;
-    ipcRenderer.send(COMPRESS_LOGS.REQUEST, toJS(logs));
+    const { fileName = filenameWithTimestamp() } = this.compressedFileDownload;
+


@daniloprates we don't need this empty line. Please remove it.

nikolaglumac · 2018-07-04T10:58:22Z

source/renderer/app/stores/ProfileStore.js

+      fileName: filenameWithTimestamp()
+    };
+    this.isCompressing = true;
+


@daniloprates we don't need this empty line. Please remove it.

thedanheller · 2018-07-04T13:48:42Z

Generating the logs only upon form submission allowed for a much simpler logic 🙂

…logs-handling

nikolaglumac

@daniloprates great improvements!
Just a couple of more small changes and then we are done:

please update log file timestamp format (posted comment in code)
now with your awesome deletion of the log files on app-start-time we no longer need to delete them on report-dialog close action (as it could cause race conditions)
one extra thing I think we need to do is to prevent closing of the report-dialog in case of an active submission (both on "X" button and on click outside of the dialog)
Thanks for implementing these!

nikolaglumac · 2018-07-05T10:12:39Z

source/common/fileName.js

+import moment from 'moment';
+
+export const filenameWithTimestamp = (prefix:string = 'logs', filetype:string = 'zip') =>
+  `${prefix}-${moment.utc().format('YYYY-MM-DDThhmmss.0SSS')}.${filetype}`;


@daniloprates please use the following format YYYY-MM-DDTHH:mm:ss.0SSS here (and this should be reflected on the getFilenameWithTimestamp function.

Oh one thing I forgot - please append Z to the end of the timestamp so that it looks like this: logs-2018-07-04T08:48:19.0182Z.zip
Thanks! :)

Hey @nikolaglumac!

I'll add the 'Z', however, it's not possible to add the colons, as it's an invalid character for filenames.

nikolaglumac · 2018-07-06T09:52:40Z

source/renderer/app/components/profile/bug-report/BugReportDialog.js

@@ -236,6 +236,11 @@ export default class BugReportDialog extends Component<Props, State> {
    this.setState({ showLogs: value });
  };

+  onClose = () => {
+    const { error, onCancel } = this.props;
+    if (!this.state.isSubmitting || error) onCancel();


@daniloprates closing should be possible in case of an error - we should prevent it only in case of submitting state!

nikolaglumac · 2018-07-06T09:52:46Z

source/renderer/app/components/profile/bug-report/BugReportDialog.js

-        onClose={onCancel}
-        closeButton={<DialogCloseButton onClose={onCancel} />}
+        onClose={this.onClose}
+        closeButton={<DialogCloseButton disabled={isSubmitting && !error} onClose={this.onClose} />}


@daniloprates closing should be possible in case of an error - we should prevent it only in case of submitting state!

nikolaglumac · 2018-07-06T09:53:52Z

@daniloprates can you please post screenshots of how the Dialog close button looks while disabled?
I think @a-rukin should take a look at it ;)

thedanheller · 2018-07-06T15:09:04Z

@nikolaglumac @a-rukin ✔︎

…logs-handling

alexander-rukin · 2018-07-08T15:49:19Z

Close button in disable state - OK

…logs-handling

thedanheller · 2018-07-10T16:49:05Z

Commits 27fc619 and 43a75fc:

Fixed bug where, when submitting the Bug Report form without filling the inputs, the button would get disabled;
Requesting the logs 2 times, when loading the BugReport dialog AND when submitting it;
Moved the isSubmitting logic into the ProfileStore;
Renamed requestFreshCompressedLogs to getLogsAndCompress;
Renamed compressedFileDownload.inProgress to compressedLogsFile.downloadInProgress;
Created bugReportInProgress;
Renamed getFileNameWithTimestamp to isFileNameWithTimestamp;
Created extractFileNameFromPath to get the fileName from the log's path.

nikolaglumac

Brilliant work @daniloprates 🎉
This is an amazing improvement and I can see how this will eliminate all the logs-related issues we had 👍

[DDW-248] Refactor compress/download logs handling

ffd61bd

thedanheller self-assigned this Jun 26, 2018

thedanheller added WIP chore labels Jun 26, 2018

thedanheller added 4 commits June 27, 2018 11:15

[DDW-248] Saving with the same timestamp

3f4af8c

[DDW-248] Removing logs on init

88b9c50

[DDW-248] Get fresh logs when submitting a support request

c8f2a81

[DDW-248] CHANGELOG

15c2a1e

thedanheller requested review from nikolaglumac and DominikGuzei July 2, 2018 12:51

nikolaglumac removed the WIP label Jul 2, 2018

nikolaglumac suggested changes Jul 4, 2018

View reviewed changes

nikolaglumac added the WIP label Jul 4, 2018

nikolaglumac reviewed Jul 4, 2018

View reviewed changes

thedanheller added 4 commits July 4, 2018 10:39

[DDW-248] UTC timestamp and FLOW

5976494

[DDW-248] Requesting logs only upon form submition

e39f322

[DDW-248] Removes useless extra lines

2d5a251

[DDW-248] Renaming getFreshLogs to avoid confusion

7ab1f8f

thedanheller added 3 commits July 4, 2018 10:49

Merge branch 'develop' into chore/ddw-248-refactor-compress-download-…

afd1343

…logs-handling

[DDW-248] Flow errors

242710e

[DDW-248] lint error

81a444d

nikolaglumac suggested changes Jul 5, 2018

View reviewed changes

thedanheller added 2 commits July 5, 2018 10:38

[DDW-248] Adjustments

9a3a123

[DDW-248] linting

344d5d5

thedanheller removed the WIP label Jul 5, 2018

nikolaglumac reviewed Jul 6, 2018

View reviewed changes

nikolaglumac added the WIP label Jul 6, 2018

thedanheller added 2 commits July 6, 2018 10:42

[DDW-248] getting logs on dialog open

37d9bf5

[DDW-248] Adjustments

0d42e24

Merge branch 'develop' into chore/ddw-248-refactor-compress-download-…

74ada87

…logs-handling

thedanheller removed the WIP label Jul 6, 2018

DominikGuzei and others added 2 commits July 10, 2018 10:34

Merge branch 'develop' into chore/ddw-248-refactor-compress-download-…

6913da9

…logs-handling

[DDW-248] Code style improvements

a7af4f1

nikolaglumac added the WIP label Jul 10, 2018

nikolaglumac and others added 2 commits July 10, 2018 13:38

[DDW-248] Code style improvements

af76c11

[DDW-248] Code improvements

27fc619

thedanheller force-pushed the chore/ddw-248-refactor-compress-download-logs-handling branch from af76c11 to 27fc619 Compare July 10, 2018 16:30

[DDW-248] lint warning

43a75fc

thedanheller removed the WIP label Jul 10, 2018

[DDW-248] Code style improvements

d68a724

nikolaglumac approved these changes Jul 12, 2018

View reviewed changes

nikolaglumac merged commit 077a182 into develop Jul 12, 2018

nikolaglumac deleted the chore/ddw-248-refactor-compress-download-logs-handling branch July 12, 2018 11:37

nikolaglumac added the release-0.12.0 label Oct 1, 2018

This was referenced Oct 16, 2018

Release/0.12.0 #1127

Closed

Release/0.12.0 #1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DDW-248] Refactor compress/download logs handling #995

[DDW-248] Refactor compress/download logs handling #995

thedanheller commented Jun 26, 2018 •

edited

Loading

nikolaglumac left a comment

nikolaglumac Jul 4, 2018

thedanheller Jul 4, 2018

nikolaglumac Jul 4, 2018

thedanheller Jul 4, 2018

nikolaglumac Jul 4, 2018

thedanheller Jul 4, 2018

nikolaglumac Jul 4, 2018

thedanheller Jul 4, 2018

nikolaglumac Jul 4, 2018

thedanheller Jul 4, 2018

nikolaglumac Jul 4, 2018

thedanheller Jul 4, 2018

nikolaglumac Jul 4, 2018

nikolaglumac Jul 4, 2018

thedanheller commented Jul 4, 2018

nikolaglumac left a comment

nikolaglumac Jul 5, 2018

nikolaglumac Jul 5, 2018

thedanheller Jul 5, 2018

thedanheller Jul 5, 2018

nikolaglumac Jul 6, 2018

nikolaglumac Jul 6, 2018

nikolaglumac commented Jul 6, 2018

thedanheller commented Jul 6, 2018

alexander-rukin commented Jul 8, 2018

thedanheller commented Jul 10, 2018 •

edited

Loading

nikolaglumac left a comment

[DDW-248] Refactor compress/download logs handling #995

[DDW-248] Refactor compress/download logs handling #995

Conversation

thedanheller commented Jun 26, 2018 • edited Loading

Todo:

Screenshots:

Normal state

Submitting state (Close button is disabled, 50% opacity)

Review Checklist:

Basics

Code Quality

Testing

After Review:

nikolaglumac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedanheller commented Jul 4, 2018

nikolaglumac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolaglumac commented Jul 6, 2018

thedanheller commented Jul 6, 2018

alexander-rukin commented Jul 8, 2018

thedanheller commented Jul 10, 2018 • edited Loading

nikolaglumac left a comment

Choose a reason for hiding this comment

thedanheller commented Jun 26, 2018 •

edited

Loading

thedanheller commented Jul 10, 2018 •

edited

Loading