-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Added Event Listener system #212
Conversation
… missing current shops table
@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, 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 Thanks for your time 🙂 |
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.
This looks good @nahid .
The last thing to do before I merge is can you create a wiki article that outlines how to use the feature and the available events.
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 |
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 |
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. 🙏 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 |
@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