-
Notifications
You must be signed in to change notification settings - Fork 87
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
Make printful syncing async #800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. One question inline...
if (!shopId) return | ||
|
||
await Shop.update( | ||
{ printfulSyncing: status, hasChanges }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the column "hasChanges" indicate whether or not the shop has config changes and needs to be re-published?
If yes, I would assume that only after a sync completed and if any produyct has been change then the hasChanges flag should be set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasChanges is passed as an arg to this functions. Yeah, I set it to true only when the job has completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm still confused... hasChanges is passed as false on line 29 below and not passed (undefined) on line 32, 36, 39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch. It should true
on line 29. Will change it.
As for the other three, Sequelize will ignore fields that are undefined
by default. You'll have to use null
if you want to unset a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I would think that the only time hasChange should be set to true is when a sync is completed.
We never want to set it to false, since it could have been set to true due to a completely separate config change that requires the shop to get re-published. Only the publish method should ever set hasChanges to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside my remaining comment about how shops.hashChanges gets updated.
|
||
if (!shopId) return | ||
|
||
await Shop.update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I did not realize sequelize does not update a column if the value is undefined... for code readability purposes, perhaps add a comment to mention this explicitly, or do not pass hasChanges to the update method if it's undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have made this change :)
* Make printful syncing async * prettify * Fix hasChanges * Address franck's comments
Related to #796
Pushes printful manual sync requests to the queue and keeps track of the status through a DB column. The status is long-polled with a 5s duration on the frontend.