Uh oh!
There was an error while loading. Please reload this page.
Scheduler Stage 3 - #488
Conversation
Fix linting from Prettier get initial cron mode id from query Add login button to preview modal Move getInitialCronMode to cron-helpers Fix linting from Prettier get initial cron mode id from query Add login button to preview modal Move getInitialCronMode to cron-helpers
| import ReactDataGrid from 'react-data-grid'; | ||
| import ms from 'ms'; | ||
| import matchSorter from 'match-sorter'; | ||
| import * as prettyCron from 'prettycron'; |
There was a problem hiding this comment.
let's use something lighter that doesn't depend on later: https://www.npmjs.com/package/cronstrue
| const initialModeId = getInitialCronMode(props.query); | ||
| const canEdit = this.props.currentRequestor && this.props.currentRequestor === props.query.requestor; |
| .simulate('click'); | ||
| // onSubmit is asynchronous | ||
| await wait(); |
There was a problem hiding this comment.
if i remember correctly, our webpack/babel/mocha setup isn't ready to use await.
There was a problem hiding this comment.
Hmm. This worked for me though. Should i remove it and use setTimeout directly instead?
There was a problem hiding this comment.
Please, ignore my comment. It doesn't apply any longer. We upgraded to Node 8 recently.
| }, | ||
| "dependencies": { | ||
| "alasql": "^0.4.5", | ||
| "cronstrue": "^1.31.0", |
| }); | ||
| }); | ||
| it('should throw GraphQL errors on failure', () => { |
| }); | ||
| describe('Creating Queries', () => { | ||
| it('should handle success cases', () => { |
There was a problem hiding this comment.
how about should schedule a query only if logged in?
| }); | ||
| describe('Updating Queries', () => { | ||
| it('should handle success cases', () => { |
There was a problem hiding this comment.
how about should update a scheduled query only if logged in?
| }); | ||
| }); | ||
| it('should throw GraphQL errors on failure', () => { |
n-riesco
commented
Jul 13, 2018
Looks good so far. I'd suggest the following changes:
How's the work on the backend progressing? |
| const spy = sinon.spy(); | ||
| const refreshInterval = 1; // second | ||
| const refreshInterval = 5; // 5 seconds | ||
| const cronInterval = `*/${refreshInterval} * * * * *`; |
There was a problem hiding this comment.
we should keep the units for testing the old format (i.e. undefined cronInterval).
There was a problem hiding this comment.
The problem with keeping refreshInterval is that it is handled differently in the queryScheduler.scheduleQuery call. Currently, refresh intervals are mapped to their corresponding cron expression before being scheduled. If they match one of the supported values, they're converted accordingly, but if they're not recognized as a supported value (as is the case of the value passed in the test), they default to a schedule of every week.
But I can update the code to accept a small enough refreshInterval to test if you'd like? I had assumed you wouldn't want me to alter the desired logic of the scheduler solely to conform to an existing test, which is why I originally opted to update the test units instead.
There was a problem hiding this comment.
On second thought, I updated the tests to use fake timers which eliminates this issue. The original refereshInterval format has been restored.
nicolaskruchten
commented
Jul 19, 2018
Cool. Just glancing at the code, there's a |
briandennis
commented
Jul 19, 2018
It shows up after taking more than 5 seconds to save/update. If you're trying to test it, we've been adding wait functions to our queries to force longer runtime. |
| "pg-hstore": "^2.3.2", | ||
| "restify": "^4.3.2", | ||
| "sequelize": "^3.30.4", | ||
| "sequelize": "3.30.4", |
| "bunyan": "^1.8.12", | ||
| "chai": "^4.1.2", | ||
| "chromedriver": "^2.33.2", | ||
| "chromedriver": "2.33.2", |
There was a problem hiding this comment.
Similar to the situation with sequelize, Chromedriver pulled in a minor patch which was throwing on CI (for example here). Freezing the version at 2.33.2 resolved the error.
n-riesco
commented
Jul 23, 2018
|
briandennis
commented
Jul 23, 2018
@n-riesco re the circleCI failure: I tried rebuilding a few times and it continued to break on that specific Redshift test with that error. I then tried connecting to redshift manually and also hit the column "typarray" does not exist in pg_type error while connecting. After some googling, it looks like this was a backwards compatibility issue with the Postgres version Redshift uses under the hood. The issue was introduced in a minor version of Freezing the version at |
briandennis
commented
Jul 23, 2018
@n-riesco the weekday input initialization has been fixed! I think this PR is at a stage where it's ready to be merged into master. Keep in mind, we're doing at least one more PR after this that addresses any UX updates and bug fixes found while doing final polish and testing before the code freeze. I think we should handle any additional changes there. What do you think? Are we good to merge this in? |
| </Row> | ||
| {ModeComponent ? <ModeComponent onChange={this.props.onChange} /> : null} | ||
| {ModeComponent ? ( | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
if the initialTime is null, we want to avoid overriding the default props for the mode by passing undefined. Setting anything to undefined however causes a lint error.
There was a problem hiding this comment.
I'll defer the decision onthis one to @nicolaskruchten .
There was a problem hiding this comment.
I'm OK with this, perhaps with a comment above the disable one to explain the reason, as it's not obvious.
| } | ||
| export function mapCronToHourFormat(cronExpression) { | ||
| if (!cronExpression) { |
There was a problem hiding this comment.
How about validating the cron expressions using cronstrue (untested)?
try{cronstrue.toString(cronExpression);}catch(_){returnnull;}There was a problem hiding this comment.
awesome idea! I added that in
n-riesco
commented
Jul 23, 2018
I'll have another go at testing the last changes tomorrow, but the code looks good to me other than the use of See also the suggestion that |
nicolaskruchten
commented
Jul 24, 2018
OK, let's merge this in and have all new changes go into a cleanup commit. Perhaps in the future when doing multi-stage work like this it would make sense to create a new branch to represent the whole feature and then do small PRs into that branch and then one final review on the branch before it hits master? |
nicolaskruchten
commented
Jul 24, 2018
(Could we get the tests to pass first though?) |
(Moved PR #487 here)
This is a WIP PR that adds
cronIntervalsupport to the frontend 👍We will shortly add the backend support and merge it in with this branch. We just wanted to get this up for @n-riesco to review before doing so.
TODO: