Drop Spree::Core::TokenResource #4736
Conversation
|
Might be some extension that could use that? Other then that 👍 |
|
Merged in my master, when CI is green I will merge it here. |
|
Thanks Peter I still need to add a migration before it reaches master though Washington L Braga Jr
|
|
ah, should have read it better, Reverted the commit!Peter Berkenbosch On 27 May 2014 at 13:57:10, Washington L Braga Jr (notifications@github.com) wrote: Thanks Peter I still need to add a migration before it reaches master though Washington L Braga Jr
|
|
Is the last thing for this just a migration for moving the existing spree_tokenized_permissions to orders and dropping the table? I want to change the saving of in progress orders from using the order_id to the token. I'm thinking calling it guest_token is a bit more specific in naming what the token is for as well. I can make the remaining changes if you guys are alright with it. In the spree_favorites extension written for Surfdome we use a guest_token https://github.com/JDutil/spree_favorites/blob/master/app/controllers/spree/favorites_controller.rb#L20-L31 to track favorites created by guest users. We'd like to standardize on the use of a guest_token to track anything a guest user is doing with a single cookie rather than multiple cookies for guest_token, order_id etc... For analytics it will be important to know everything being done by the same user rather than creating unique tokens for each different thing. |
|
👍 Thanks @JDutil looks good! |
|
@JDutil I don't intend to drop that table now, we'd better leave it there and drop on some other release. Should add the migration soon merge and you could go from there sounds good? I'm just not sure I understand what you mean by I'm thinking calling it guest_token is a bit more specific in naming what the token is for as well. isn't that token also used by api to change stuff in orders? Trying to understand why calling it guest_token would make sense. Also isn't it already possible to track anything any user can do with the last session to cookie change? thanks for jumping in |
|
@huoxito I had tried writing a migration to migrate the data last night and kept getting different syntax errors so I gave up anyways... As for the token currently a request coming in with the token parameter will set session[:access_token]. I'd prefer to call it guest token and have the token refer to the same guest everywhere by making it a cookie rather than session variable. Session variables are lost anytime the user closes their browser, and can result in the same user generating many orders if they just browse around and don't finish their orders. If we set within a cookie then the user is able to continually keep coming back to their same order until it is completed whether they have closed their web browser or not. Yes the last change I made works in tracking the order id across sessions, but the idea is to take it a step further. It would no longer use the order id, but instead use the guests token. The guests token would remain the same across each thing you want to track about the user so that you can tell that it is the same guest user in analytics. For example if you have the same user with a partial order, and adding favorites as a guest it would be useful to be able to link that information together in your analytics and recommendations. So it would be easiest to do this by deciding upon a standard such as cookie[:guest_token] that people can build off from. Does that make sense? |
|
Yes! Love the sound of that, thanks for taking the time to explain. I never noticed that session access_token key tbh. Also just to make sure I got it right. We'd still have a Order#token attribute but we'd have a guest_token cookie is that right? Or are you saying we should call it Order#guest_token in the model / table? Either I'll try to wrap up the migration today so we can merge it. |
None of spree components use that feature except for the token attribute itself which we could just move to Order so we can run less things when creating an object
|
Guys migration added, looks accurate to me but I'd appreciate if anyone could double check it |
|
@huoxito I think we should make it just the guest_token so it's consistent for all use. |
|
@huoxito I'm thinking we would make it Order#guest_token for the database, but will keep the parameter in the urls simply token. I can make the changes necessary for that building off this pr if there are no objections |
|
amended to fix the typo. @JDutil yes please feel free to work from here. Sorry I didn't update the column name, there should be only a couple places to change on this PR though. I guess we close this one once you open another one with all the cookie stuff? thanks Jeff!! |
|
This should be resolved once #4816 is merged |
|
Just saw this change. I love it :) 👍 |
None of spree components use that feature except for the token attribute
itself which we could just move to Order so we can run less things when
creating an object
Been trying to review all queries executed while creating and completing orders (orders.contents.add still run a ton of stuff I think we should try harder to improve) and noticed this tokenized_permission table. Does anyone see a good reason to keep it in spree core? This is still missing a migration, will add it in case we agree this association should be dropped.