reverbdotcom / reverb-magento

Magento 1.x plugin for syncing with Reverb
Other
7 stars 10 forks source link

#251 Creating orders when an update comes in for an order which has n… #256

Closed dunagan5887 closed 8 years ago

dunagan5887 commented 8 years ago

Fix #251 This has been tested and should be good to go

skwp commented 8 years ago

Are people going to have leftover order creation tasks in their grid that is now getting renamed to shipment tracking? Do we need a migration to move those? What will happen to the existing queued tasks?

dunagan5887 commented 8 years ago

Existing queue tasks will stay in the table, but they will not appear in the grid. If we want them deleted then we will need a migration to delete them. Should I implement this?

skwp commented 8 years ago

Let's leave as is, as long as they dont' appear in the grid that will be ok.

skwp commented 8 years ago

Will they finish processing as normal though?

dunagan5887 commented 8 years ago

No, they will not finish processing as normal. I deactivated the order creation cron since orders are now only being created via order updates. Only when an order update comes in will orders be created

skwp commented 8 years ago

Won't this affect existing customers? If they have order creation tasks in the middle of executing then when they install this release those tasks will never finish?

dunagan5887 commented 8 years ago

Any tasks which are in the middle of executing will still be executed. Additionally, there may be some order_creation rows left in the cron_schedule database table which would cause the order creation cron job to be executed several more times, but there is no guarantee that these tasks would be executed

skwp commented 8 years ago

Is it better to make a migration that moves those creation takes to update tasks perhaps?

zztimur commented 8 years ago

What's the plan, guys?

On Fri, Jun 10, 2016 at 3:37 PM, Yan Pritzker notifications@github.com wrote:

Is it better to make a migration that moves those creation takes to update tasks perhaps?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/reverbdotcom/reverb-magento/pull/256#issuecomment-225289348, or mute the thread https://github.com/notifications/unsubscribe/AAmFMgKUr3salNh3DsD6sxlxosoukwg1ks5qKcsfgaJpZM4IzPbx .

-- Sent from my iPhone

dunagan5887 commented 8 years ago

We could make a migration to move those creation tasks to update tasks, but I'm not sure why we would want to do that. Once the order update comes in the orders will be created.

In terms of a migration, it seems to me that it would make the most sense to delete all order creation tasks which have status us COMPLETE or ABORT. This would prevent the deletion of tasks which are currently being processed, but would potentially result in some order creation tasks remaining in the table.

If we want to prevent creation tasks from remaining in the table, we may need to tell clients to disable their Magento crontabs, wait for any currently processing cron jobs to finish, upgrade the Reverb extension, and then re-enable the Magento crontab

kylecrum commented 8 years ago

@dunagan5887 I'd err on the side of requiring clients not to do specific things to upgrade, because usually people don't follow those instructions and just install.

Is there an issue with leaving some creation tasks remaining in the table? As you said, it seems like the update task would do the creation anyway.

dunagan5887 commented 8 years ago

Aside from taking up some space in the database, there wouldn't be a functional issue with this. I can't imagine the space would be noticeable

kylecrum commented 8 years ago

I'm leaning towards this then:

it seems to me that it would make the most sense to delete all order creation tasks which have status us COMPLETE or ABORT. This would prevent the deletion of tasks which are currently being processed, but would potentially result in some order creation tasks remaining in the table.

dunagan5887 commented 8 years ago

Sounds good, I should be able to get to this either today or tomorrow

dunagan5887 commented 8 years ago

I fixed the merge conflict, should be ready to go