Skip to content
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

Add Events support for Command #11064

Merged
merged 8 commits into from Jul 13, 2023
Merged

Add Events support for Command #11064

merged 8 commits into from Jul 13, 2023

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Jun 19, 2023

🎩 What? Why?

In the review of #10151, @ahukkanen suggested to use a hook method in the commands so that we can easily issue ActiveSupport Notifications that can be subscribed to from various places of Decidim application. While implementing this, i have seen that we already have some other events that we dispatch, so, in order to have a consolidated mode of issuing application wide events, i have created this PR.

📌 Related Issues

Link your PR to an issue

Testing

Follow the test scenarios in #6804 and #10111

♥️ Thank you!

@alecslupu alecslupu changed the title Refactor event system Add Events support for Commands Jun 19, 2023
@alecslupu alecslupu changed the title Add Events support for Commands Add Events support for Command Jun 19, 2023
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have gone through the code as well as tested this by adding the following initializer code in the development application:

log_path = Rails.root.join("log/testing_events.log")
logger = Logger.new(log_path)

logger.info("Started the application!")

Decidim::EventsManager.subscribe("decidim.admin.block_user:before") do |event_name, data|
  logger.info("Block user -- BEFORE")
  logger.info(event_name.inspect)
  logger.info(data.inspect)
end

Decidim::EventsManager.subscribe("decidim.admin.block_user:after") do |event_name, data|
  logger.info("Block user -- AFTER")
  logger.info(event_name.inspect)
  logger.info(data.inspect)
end

It worked great and logged both events when I blocked a user.

Overall I really like the approach, I have just left a couple of comments regarding some code clarification and regarding the with_events method itself which I think could be simplified.

RELEASE_NOTES.md Outdated Show resolved Hide resolved
RELEASE_NOTES.md Outdated Show resolved Hide resolved
decidim-core/lib/decidim/command.rb Outdated Show resolved Hide resolved
decidim-core/lib/decidim/command.rb Outdated Show resolved Hide resolved
alecslupu and others added 2 commits July 12, 2023 13:20
Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi>
@alecslupu alecslupu requested a review from ahukkanen July 12, 2023 18:39
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more remark and this is good to go for me.

There may be a reason for this but I didn't understand it.

@alecslupu alecslupu requested a review from ahukkanen July 13, 2023 11:20
@ahukkanen ahukkanen merged commit 0f44db7 into develop Jul 13, 2023
94 of 96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants