Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

[WIP] final query scheduling fixes and testing #500

Merged
merged 26 commits into from
Jul 27, 2018
Merged

[WIP] final query scheduling fixes and testing #500

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
bf2d0d1
Disallow editing query name after save
mfix22 Jul 24, 2018
044a866
add log-in warning message to scheduler tab
mfix22 Jul 24, 2018
4771514
enhance login before scheduling a query message
mfix22 Jul 24, 2018
842dd96
fix linting
mfix22 Jul 24, 2018
db76145
add more comprehensive scheduling testing
briandennis Jul 25, 2018
601196c
correct monthly cron interval test comment
briandennis Jul 25, 2018
a13fd98
remove queries won't run warning, add info if logged in as different …
briandennis Jul 25, 2018
96005d4
Minor style fixes
jakedex Jul 25, 2018
d696973
Wrap lines on CreateModal
jakedex Jul 25, 2018
f8e7485
Fix overflow on Schedule and Query pages
jakedex Jul 25, 2018
aa4548a
'Interval' -> 'Schedule' in scheduled table
jakedex Jul 25, 2018
2ba6cee
Add text-overflow for long interval strings
jakedex Jul 25, 2018
fa05078
Misc modal style fixes
jakedex Jul 25, 2018
631d4a6
Fix scheduler double border
jakedex Jul 25, 2018
82c4d56
bump react-chart-editor to 0.21
briandennis Jul 25, 2018
f53ddfc
revert login href change
mfix22 Jul 25, 2018
ddd18e1
Add query name validation
jakedex Jul 25, 2018
0bf0b95
Linting/cleanup
jakedex Jul 25, 2018
75cb763
Copy query to clipboard
jakedex Jul 25, 2018
01572d5
Fix close button position, lint
jakedex Jul 25, 2018
d20fc70
fix preview prop type
briandennis Jul 26, 2018
187aad4
bump react-chart-editor version to 0.21.1
briandennis Jul 26, 2018
2817b2a
add UI signal that query was copied
briandennis Jul 26, 2018
00ce988
trim query name on submit without erroring
briandennis Jul 26, 2018
8c92585
make preview prop optional
briandennis Jul 26, 2018
391bd5a
remove whitespace only error
briandennis Jul 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/components/Settings/scheduler/create-modal.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
width: 80%;
}

.create-modal .row-body pre {
.create-modal .row-body > pre {
margin-top: 5px;
overflow-y: auto;
max-height: 300px;
}
24 changes: 17 additions & 7 deletions app/components/Settings/scheduler/create-modal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class CreateModal extends Component {
saving: false
};
this.options = {
lineWrapping: true,
lineNumbers: true,
tabSize: 4,
readOnly: false,
Expand Down Expand Up @@ -103,6 +104,9 @@ class CreateModal extends Component {
if (!this.state.interval) {
return this.setState({error: 'Please select an interval above.'});
}
if (this.state.name && this.state.name.trim().length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't debugged the reason, but this isn't working for me. I'm still able to create queries without a name:

peek 2018-07-26 10-50

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s by design I think...

Copy link
Contributor

Choose a reason for hiding this comment

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

Er... perhaps not, looking at the code. I should have said “I had assumed it was by design that you can create queries without names and it didn’t bother me”

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it looks like you can create queries named "" but not " "... The first part of the if condition is falsey for "". Given that we still have to support unnamed queries, we may as well just trim and move on if the user tries " " instead of complaining :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@briandennis How about if ((this.state.name || '').trim().length === 0)?

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference here would be to allow any trimmed string, even empty ones, but I'm also OK with the current behaviour if we're out of time.

return this.setState({error: 'Please enter a valid query name above.'});
}

this.setState({saving: true, error: null});
this.props
Expand All @@ -122,7 +126,7 @@ class CreateModal extends Component {
render() {
return (
<Modal open={this.props.open} onClickAway={this.props.onClickAway} className="scheduler create-modal">
<Column className="container" style={{width: '60%', maxHeight: '100vh', minWidth: 640}}>
<Column className="container" style={{width: '60%', maxHeight: '100vh', minWidth: 640, paddingBottom: '16px'}}>
<Row>
<Column className="innerColumn">
<h5 className="header">Create Scheduled Query</h5>
Expand Down Expand Up @@ -152,12 +156,18 @@ class CreateModal extends Component {
<Row style={secondaryRowStyle}>
<div className="row-header">Query name</div>
<div className="row-body">
<input
maxLength="150"
placeholder="Enter query name here..."
value={this.state.name}
onChange={this.handleNameChange}
/>
{this.state.successMessage ? (
<em style={{marginTop: 5, display: 'inherit'}}>
<b>{this.state.name}</b>
</em>
) : (
<input
maxLength="150"
placeholder="Enter query name here..."
value={this.state.name}
onChange={this.handleNameChange}
/>
)}
</div>
</Row>
<Row style={secondaryRowStyle}>
Expand Down
5 changes: 4 additions & 1 deletion app/components/Settings/scheduler/login-modal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ const PromptLoginModal = props => (
&times;
</button>
<Row className="header">
<p>To create a scheduled query, you'll need to be logged into Plotly.</p>
<p>
To create a scheduled query, you'll need to be logged into Plotly. Logging in will reset your query,
so please save it elsewhere before doing so.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably about as good as we can do I suppose. There's no way to squirrel the query away into local storage or a cookie or something and restore it after login?

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to look into this on the next stage, but given time constraints, I don't feel comfortable implementing/testing this functionality before the release.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Any way we could make this warning very clear? put it on its own line, in bold? this is a pretty bad failure case if you've just iterated for an hour on some complex query.

</p>
</Row>
<Row>
<button type="submit" className="submit" onClick={props.onSubmit}>
Expand Down
40 changes: 32 additions & 8 deletions app/components/Settings/scheduler/preview-modal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ export class PreviewModal extends Component {
if (this.state.editing) {
const {connectionId, fid, requestor, uids, refreshInterval} = this.props.query;
const {code: query, cronInterval, name} = this.state;

if (this.state.name && this.state.name.trim().length === 0) {
return this.setState({error: 'Please enter a valid query name above.'});
}

this.setState({loading: true, error: null});
this.props
.onSave({
Expand Down Expand Up @@ -130,14 +135,32 @@ export class PreviewModal extends Component {

renderButtonRow() {
const {loading, editing} = this.state;
const loggedIn = this.props.currentRequestor;
const canEdit = this.props.currentRequestor && this.props.currentRequestor === this.props.query.requestor;
const success = this.state.successMessage;

if (!canEdit) {
return (
<button style={noMargin} onClick={this.props.onLogin}>
Log in to edit query
</button>
<React.Fragment>
{loggedIn ? (
<Column>
<Row>
<p style={{fontSize: 12, marginBottom: '16px', opacity: 0.7, width: '100%'}}>
This query was created by another user. To modify, please log in as that user.
</p>
</Row>
<Row style={{justifyContent: 'flex-start'}}>
<button style={noMargin} onClick={this.props.onLogin}>
{loggedIn ? 'Switch users' : 'Log in to edit query'}
</button>
</Row>
</Column>
) : (
<button style={noMargin} onClick={this.props.onLogin}>
Log in to edit query
</button>
)}
</React.Fragment>
);
}

Expand Down Expand Up @@ -189,11 +212,12 @@ export class PreviewModal extends Component {
const initialModeId = getInitialCronMode(props.query);

content = (
<Column style={{width: '60%', maxHeight: '100vh', minWidth: 640, background: 'white'}}>
<Column style={{width: '60%', maxHeight: '100vh', minWidth: 640, background: 'white', paddingTop: 16}}>
{ editing && <Row style={{ padding: '0 32px', justifyContent: 'flex-start', fontSize: 12, marginTop: 8, fontWeight: 600, opacity: 0.4, letterSpacing: '0.5px' }}>EDITING</Row> }
<Row
className="sql-preview"
style={{
padding: '16px 32px',
padding: '0 32px 16px',
position: 'relative',
justifyContent: 'flex-start'
}}
Expand All @@ -216,9 +240,9 @@ export class PreviewModal extends Component {
<Column style={{background: '#F5F7FB', padding: '16px 32px'}}>
<Row style={rowStyle}>
<div style={keyStyle}>Query</div>
<div className="sql-preview scheduler" style={{...valueStyle, overflowY: 'auto'}}>
<div className="sql-preview scheduler" style={{...valueStyle, overflowY: 'auto', maxHeight: 300}}>
{editing ? (
<div>
<div style={{ width: '99%' }}>
<CodeMirror
options={{
lineNumbers: true,
Expand Down Expand Up @@ -305,7 +329,7 @@ export class PreviewModal extends Component {
justifyContent: 'space-between',
border: 'none',
marginTop: 48,
paddingBottom: 0
paddingBottom: 16
}}
>
{this.renderButtonRow()}
Expand Down
7 changes: 1 addition & 6 deletions app/components/Settings/scheduler/scheduler.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,9 @@
}

.scheduler-table .react-grid-Row {
border-top: 1px solid #c8d4e3;
cursor: pointer;
}

.scheduler-table .react-grid-Row:first-child {
border-top: none;
}

.sql pre {
margin: 0 auto;
max-height: 100%;
Expand All @@ -79,5 +74,5 @@
}

.meta-preview button {
min-height: 28px;
min-height: 21px;
}
30 changes: 14 additions & 16 deletions app/components/Settings/scheduler/scheduler.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,19 @@ class IntervalFormatter extends React.Component {
const run = this.props.value;

return (
<Row>
<Column>
<em
style={{
fontSize: 15
}}
>
{run.cronInterval
? cronstrue.toString(run.cronInterval)
: `Runs every ${ms(run.refreshInterval * 1000, {
long: true
})}`}
</em>
</Column>
</Row>
<em
className="ellipsis"
style={{
display: 'block',
fontSize: 15
}}
>
{run.cronInterval
? cronstrue.toString(run.cronInterval)
: `Runs every ${ms(run.refreshInterval * 1000, {
long: true
})}`}
</em>
);
}
}
Expand Down Expand Up @@ -141,7 +139,7 @@ class Scheduler extends Component {
},
{
key: 'run',
name: 'Interval',
name: 'Schedule',
filterable: true,
formatter: IntervalFormatter
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@
"query-string": "^5.0.1",
"ramda": "^0.21.0",
"react": "^16.2.0",
"react-chart-editor": "0.18",
"react-chart-editor": "0.21",
"react-click-outside": "^3.0.1",
"react-codemirror2": "^4.2.1",
"react-cookies": "^0.1.0",
Expand Down
19 changes: 9 additions & 10 deletions static/css/app.global.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ a:hover {
body {
background-color: #F3F6FA;
position: relative;
height: 100vh;
overflow-y: scroll;
overflow-y: auto;
font-weight: 200;
font-size: 1.5em; /* currently ems cause chrome bug misinterpreting rems on body element */
line-height: 1.6;
Expand Down Expand Up @@ -59,7 +58,7 @@ h6 { font-size: 1.5rem; line-height: 1.6; letter-spacing: 0; }

p {
margin-top: 0.5rem;
margin-bottom: 0.5rem;
margin-bottom: 0.5rem;
}

*::-webkit-input-placeholder {
Expand Down Expand Up @@ -109,40 +108,40 @@ button, .btn-primary {
}

button.btn-secondary {
color: #506784;
color: #506784;
background-color: #ebf0f8;
border: 1px solid #dfe8f3;
text-shadow: none;
}

button:hover, .btn-primary:hover {
color: #ffffff;
color: #ffffff;
background-color: #0f89df;
border: 1px solid #0f89df;
}

button.btn-secondary:hover {
background-color: #dfe8f3;
border: 1px solid #c8d4e3;
color: #2a3f5f;
color: #2a3f5f;
}

button:active, .btn-primary:active {
color: #ffffff;
color: #ffffff;
background-color: #0d76bf;
border: 1px solid #0d76bf;
}

button.btn-outline {
background: #fff;
background: transparent;
color: #119DFF;
display: inline-block;
margin-right: 20px;
text-shadow: none;
padding: 9px 18px;
font-weight: 300;
border-radius: 4px;
font-size: 12px;
font-size: 12px;
}

.dd-menu > button {
Expand Down
Loading