Closed nahid closed 8 months ago
Hi @nahid
Thanks for submitting the PR - can you please look at the actions as your PR is failing.
I've also taken a quick look at the code changes, but I don't see any new tests. Can you make sure these changes are going to be covered by codecov.
Also, you've made some changes to the installShop and the authentication, can you outline in your PR what those changes are for.
Thanks!
@Kyon147 thanks for your quick reply and queries. I just added all possible tests with this PR.
Hey @Kyon147, is there any suggestions from?
Hello @Kyon147, I hope everything is going well. It's been a while since this PR was submitted, what are your thoughts on it? Is there anything else that needs to be changed or improved?
Hey @nahid
I just need to check the event system in a blank app, so I can QA it to make sure as it is a big PR.
Hi @Kyon147 ,
I understand the need for a thorough test on this significant PR. It'll be great if you test before merge. Feel free to reach out if you encounter any issues or have questions.
Your feedback is much appreciated.
Hello @Kyon147, I hope you're doing well. I understand you're very busy and finding it difficult to spare time. I'm a power user of this package, and this PR is very important to me. I hope it will also make life easier for others. If you have the time to test the PR in your free time and have any suggestions, or if everything looks good, merging it would be very helpful.
Thank you for your excellent work on this package and for your contributions to the community.
Hey @nahid
Thanks for the ping to remind me, I'll take a look at this later today and if it looks good will merge it into master and set up a new release for it.
Thank you for taking the time to work on the PR - I'm sure it will be super helpful for the rest of the community.
Hey @Kyon147 , Sorry for disturbing you. Any updates regarding this PR?
Hey @Kyon147 , Sorry for disturbing you. Any updates regarding this PR?
Hey @nahid I've got this on my local and just running some QA on it. Should be done shortly.
Hey @Kyon147 Thanks for your time đ
Awesome, sure! I'll get started on creating that wiki article ASAP and will keep you posted once it's done, @Kyon147
BTW @Kyon147, how can I contribute to this project's wiki?
Hi @nahid
You should just be able to edit the wiki but I have created a page for you here to start off. https://github.com/Kyon147/laravel-shopify/wiki/Event-System
@Kyon147, Thanks for your support, but unfortunately I can't edit this page in any way âšī¸
@Kyon147, Thanks for your support, but unfortunately I can't edit this page in any way âšī¸
Can you try again now @nahid had to change a setting in the repo.
Great, now it works fine @Kyon147
Hello @Kyon147, Hope you're doing well. I've just finished the Event Listener wiki. Kindly review it, and please don't hesitate to share any feedback or suggest changes if necessary.
wiki: https://github.com/Kyon147/laravel-shopify/wiki/Event-Listener-System
Hello @Kyon147, Hope you're doing well. I've just finished the Event Listener wiki. Kindly review it, and please don't hesitate to share any feedback or suggest changes if necessary.
wiki: https://github.com/Kyon147/laravel-shopify/wiki/Event-Listener-System
Amazing, thanks for taking the time to sort the PR and the wiki, its appreciated. I'll merge in the work and get a new release out this week đ
Hello @nahid, this is a great feature. Thanks a lot for adding this. đ I noticed a small problem.
Context: This happens after uninstalling and installing the app again when the shop is in soft delete mode.
In this case, ShopAuthenticatedEvent is being fired when a shop is installed, not when authenticated and this is causing some problems.
This is how I defined the listener.
And this is the code inside my listener.
After uninstalling and installing the shop again, it is being fired before I actually see the permission screen on Shopify. So, at this point, my user has been soft-deleted from the DB.
And this is the log I see in the terminal.
cc: @Kyon147
Could you please share the $shop
model dump with me @faridmovsumov
@nahid using the AppInstalled event solved my problem. I made the wrong assumption by naming.
As I mentioned in @gnikyt PR 1220, I just added some events in the various cases for this package. These features help developers add more control over this package. Now developers can do anything based on the triggered event by registering their own listener/s, e.g.: send email/SMS/notification, run job, update DB, or anything else. Also, this PR includes some refactoring with performance improvements.
Events
AppInstalledEvent
ShopAuthenticatedEvent
ShopDeletedEvent
AppUninstalledEvent
PlanActivatedEvent
Backward compatibility: YES Breaking change: NO Deprecation: YES (AfterAuthenticationJob feature will be removed in the next major release) Doc: Need to update the WIKI after merging this PR with the migration guide