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.

Sign up for GitHub

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

Jump to bottom

feat(job/log): add more details on a backup #2245

Merged
merged 8 commits into from Jun 30, 2017
Merged

feat(job/log): add more details on a backup #2245

merged 8 commits into from Jun 30, 2017

Conversation

badrAZ
Copy link
Contributor

@badrAZ badrAZ commented Jun 30, 2017

Fixes #2239

@badrAZ badrAZ self-assigned this Jun 30, 2017
@badrAZ badrAZ requested a review from julien-f June 30, 2017 10:29
julien-f
julien-f requested changes Jun 30, 2017
View reviewed changes
src/xo-app/logs/index.js Outdated
@@ -41,6 +42,18 @@ const jobKeyToLabel = {
rollingSnapshot: _('rollingSnapshot')
}

const formatSize = bytes =>
Copy link
Member

Choose a reason for hiding this comment

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

Already defined in utils.

src/xo-app/logs/index.js Outdated
unit: 'B'
})

const formatSpeed = (bytes, milliseconds) =>
Copy link
Member

Choose a reason for hiding this comment

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

Move to utils.

src/xo-app/logs/index.js Outdated
@@ -70,9 +83,50 @@ class JobReturn extends Component {
}
}

class JobTransferredData extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

Simple function.

src/xo-app/logs/index.js Outdated
}

class JobCallState extends Component {
_renderInfos = () => this.props.error !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

Inline.

src/xo-app/logs/index.js Outdated
}
}

class JobCallState extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

Simple function.

pdonias
pdonias requested changes Jun 30, 2017
View reviewed changes
src/xo-app/logs/index.js Outdated
}

class JobCallState extends Component {
_renderInfos = () => this.props.error !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

Avoid calling it render* when it doesn't render a component.

src/xo-app/logs/index.js Outdated
return <div>
<span><strong>{_('jobTransferredDataSize')}</strong>: {formatSize(size)} </span>
<br />
<span><strong>{_('jobTransferredDataSpeed')}</strong>: {formatSpeed(size, end - start)} </span>
Copy link
Member

Choose a reason for hiding this comment

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

: should be internationalized with the label.

Copy link
Member

Choose a reason for hiding this comment

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

Extra spaces before </span>s.

Copy link
Member

Choose a reason for hiding this comment

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

The whole text should be i18n: use data interpolation

julien-f
julien-f requested changes Jun 30, 2017
View reviewed changes
src/xo-app/logs/index.js Outdated
? {icon: 'halted', tooltip: 'failedJobCall'}
: end !== undefined
? {icon: 'running', tooltip: 'successfulJobCall'}
: {icon: 'busy', tooltip: 'jobCallInProgess'}
Copy link
Member

Choose a reason for hiding this comment

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

With array destructuring, the code is a bit lighter:

const [ icon, tooltip ] = error !== undefined
  ? [ 'halted', 'failedJobCalled' ]
  : end !== undefined
    ? [ 'running', 'successfulJobCall' ]
    : [ 'busy', tooltip: 'jobCallInProgress' ]

src/xo-app/logs/index.js Outdated
const getJobTransferredDataInfos = (start, end, size) => <div>
<span><strong>{_('jobTransferredDataSize')}</strong> {formatSize(size)}</span>
<br />
<span><strong>{_('jobTransferredDataSpeed')}</strong> {formatSpeed(size, end - start)}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Both the label and the value should be translatable because you have no guarantee that the order will stay the same in every languages.

We'll keep this as is for now but we'll have to fix this later.

julien-f
julien-f requested changes Jun 30, 2017
View reviewed changes
src/xo-app/logs/index.js Outdated
@@ -70,9 +73,33 @@ class JobReturn extends Component {
}
}

const getJobCallStateInfos = (end, error) => {
Copy link
Member

Choose a reason for hiding this comment

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

Functional component.

src/xo-app/logs/index.js Outdated
</Tooltip>
}

const getJobTransferredDataInfos = (start, end, size) => <div>
Copy link
Member

Choose a reason for hiding this comment

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

Functional component.

julien-f
julien-f requested changes Jun 30, 2017
View reviewed changes
src/xo-app/logs/index.js Outdated
@@ -109,9 +109,9 @@ const Log = props => <ul className='list-group'>
}

return <li key={call.callKey} className='list-group-item'>
<strong className='text-info'>{call.method}: </strong>{getJobCallStateInfos(end, error)}<br />
<strong className='text-info'>{call.method}: </strong>{getJobCallStateInfos({ end, error })}<br />
Copy link
Member

Choose a reason for hiding this comment

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

getJobCallStateInfos should be React component, and named and used as such.

src/xo-app/logs/index.js Outdated
{map(call.params, (value, key) => [ <JobParam id={value} paramKey={key} key={key} />, <br /> ])}
{returnedValue != null && returnedValue.size !== undefined && getJobTransferredDataInfos({ start, end, size: returnedValue.size })}
Copy link
Member

Choose a reason for hiding this comment

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

Idem for getJobTransferredDataInfos.

@julien-f julien-f merged commit 995e76d into next-release Jun 30, 2017
@julien-f julien-f deleted the improveBackupLog branch June 30, 2017 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-f julien-f julien-f approved these changes

@olivierlambert olivierlambert olivierlambert approved these changes

@pdonias pdonias pdonias approved these changes

Assignees

@badrAZ badrAZ

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@badrAZ @julien-f @olivierlambert @pdonias