Notify users when roles get assigned by anaghavl · Pull Request #5886 · decidim/decidim

anaghavl · 2020-03-20T19:17:01Z

Notify users when roles get assigned

📌 Related Issues

Related to #?
Fixes Receive notification of the role I have been assigned #5733

📋 Subtasks

📷 Screenshots (optional)

codecov · 2020-03-20T19:28:34Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@9c7b36e). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #5886   +/-   ##
==========================================
  Coverage           ?   94.24%           
==========================================
  Files              ?     1101           
  Lines              ?    30086           
  Branches           ?        0           
==========================================
  Hits               ?    28355           
  Misses             ?     1731           
  Partials           ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c7b36e...aa9029d. Read the comment docs.

anaghavl · 2020-03-23T06:53:12Z

@mrcasals @leio10 @agustibr Please review :)

mrcasals

Hi @anaghavl! Nice job, really!

There are some minor things related to i18n and some code style issues, but apart from that code looks good! I see there are not tests though, can you check those in #4636 as an example?

Also, for the PR description we usually link the issue this PR works on. In this case, it's #5733, so can you put it in the "Fixes #?" secction? This way when we merge this PR the issue will be closed automatically 😄

Also, don't forget to add a line on the CHANGELOG file! 😄

mrcasals · 2020-03-23T07:54:43Z

decidim-assemblies/app/commands/decidim/assemblies/admin/notify_role_assigned_assembly.rb

+    module Admin
+      # A command to notify users when a role is assigned for a participatory process
+      class NotifyRoleAssignedAssembly < Rectify::Command
+        def send_notification user


Even though Ruby syntax allows this, we usually put parenthesis when defining a method:

Suggested change

def send_notification user

def send_notification(user)

mrcasals · 2020-03-23T08:02:09Z

decidim-assemblies/app/events/decidim/assembly_role_assigned_event.rb

+        resource_title: resource_title,
+        resource_url: resource_url,
+        scope: event_name,
+        role: extra["role"]


We're sending the raw value of the role to the event, and then use that value when showing the i18n string. This will result in an English mashup when using locales other than English. Maybe we could refactor this? I think it's fine sending the raw value, this way we can translate it here:

def i18n_role I18n.t(extra["role"], "decidim.admin.models.assembly_user_role.roles", default: nil) end def i18n_options { ... role: i18n_role } end def notification_title return I18n.t("notification_title", i18n_options).html_safe if i18n_role I18n.t("notification_title_general_role", i18n_options).html_safe end

And then add the notification_title_general_role with the string "You have been assigned a role to %{resource_title}" or something like that.

What do you think? 😄

This would need to be applied to all similar classes!

Mmm, maybe the user would prefer to have the non-translated role name to don't have anything. We could do something like:

def i18n_role I18n.t(extra["role"], "decidim.admin.models.assembly_user_role.roles", default: extra["role"]) end def i18n_options { ... role: i18n_role } end def notification_title I18n.t("notification_title", i18n_options).html_safe end

What do you think?

Sounds good to me!

mrcasals · 2020-03-23T08:03:28Z

decidim-conferences/app/commands/decidim/conferences/admin/notify_role_assigned_conference.rb

+    module Admin
+    # A command to notify users when a role is assigned for a Conference
+      class NotifyRoleAssignedConference < Rectify::Command
+        def send_notification user


Suggested change

def send_notification user

def send_notification(user)

mrcasals · 2020-03-23T08:03:57Z

decidim-conferences/app/commands/decidim/conferences/admin/notify_role_assigned_conference.rb

+    # A command to notify users when a role is assigned for a Conference
+      class NotifyRoleAssignedConference < Rectify::Command
+        def send_notification user
+            Decidim::EventsManager.publish(


Something funny with the identation here!

You can automatically run bundle exec rubocop -a <list of files you want to check>, or manually check the "Lint" job in the Checks section of this PR, it will tell you what's missing 😄

agustibr

😄 Good work! Please, check that the participatory_space is present in the form object.

decidim-assemblies/app/commands/decidim/assemblies/admin/update_assembly_admin.rb

decidim-assemblies/app/controllers/decidim/assemblies/admin/assembly_user_roles_controller.rb

decidim-conferences/app/commands/decidim/conferences/admin/update_conference_admin.rb

...im-conferences/app/controllers/decidim/conferences/admin/conference_user_roles_controller.rb

...ses/app/commands/decidim/participatory_processes/admin/update_participatory_process_admin.rb

...rollers/decidim/participatory_processes/admin/participatory_process_user_roles_controller.rb

leio10

Nice work! 💯 👍
You may be a bit overwhelmed by the project size, the reviews, the linting tools, the tests, and so on, but don't worry that you are doing it great and you will get used to all of that very soon. 💪

Extra tip: you can request a review using the right sidebar 😃

leio10 · 2020-03-23T10:50:52Z

decidim-assemblies/app/events/decidim/assembly_role_assigned_event.rb

+        resource_title: resource_title,
+        resource_url: resource_url,
+        scope: event_name,
+        role: extra["role"]


Mmm, maybe the user would prefer to have the non-translated role name to don't have anything. We could do something like:

def i18n_role I18n.t(extra["role"], "decidim.admin.models.assembly_user_role.roles", default: extra["role"]) end def i18n_options { ... role: i18n_role } end def notification_title I18n.t("notification_title", i18n_options).html_safe end

What do you think?

leio10

Great work!

anaghavl · 2020-03-31T10:01:21Z

Hey @decidim/core this PR is failing due to codecov checks, but we’re not sure how to fix them, do you have any insight?

agustibr

It seems that emails are not sent to users (if user has enabled "Send notifications by email"), can you check it? 😀

jesusdb · 2020-04-01T10:20:04Z

Hello @anaghavl, the Codecov failing checks mean that there is code not covered by tests. To see what parts are uncovered, you can click on Details and look at "COVERAGE REACH"; the red squares mean the file needs more coverage. If you can't see any red square, you can click on "Diff" tab, and the first column from the files indicates if it's covered (green) or not (red).

mrcasals · 2020-04-14T12:24:40Z

@anaghavl there are some conflicts on the changelog, can you check them please? 😄

tramuntanal

Hi @anaghavl good job!
I've just suggested some renamigs for clarification 😄

tramuntanal · 2020-04-23T14:14:36Z

decidim-assemblies/app/events/decidim/assembly_role_assigned_event.rb

+# frozen-string_literal: true
+
+module Decidim
+  class AssemblyRoleAssignedEvent < Decidim::Events::SimpleEvent


Suggested change

class AssemblyRoleAssignedEvent < Decidim::Events::SimpleEvent

class RoleAssignedToAssemblyEvent < Decidim::Events::SimpleEvent

tramuntanal · 2020-04-23T14:17:34Z

decidim-assemblies/app/commands/decidim/assemblies/admin/notify_role_assigned_assembly.rb

+  module Assemblies
+    module Admin
+      # A command to notify users when a role is assigned for a participatory process
+      class NotifyRoleAssignedAssembly < Rectify::Command


Let's add the preposition, this will affect the class and the file names:

Suggested change

class NotifyRoleAssignedAssembly < Rectify::Command

class NotifyRoleAssignedToAssembly < Rectify::Command

also the documentation for this class uses "a participatory process" when it should say "an assembly", correct?

nice catch, did not realise, fixed it :)

tramuntanal · 2020-04-23T14:35:09Z

decidim-conferences/app/commands/decidim/conferences/admin/notify_role_assigned_conference.rb

+  module Conferences
+    module Admin
+      # A command to notify users when a role is assigned for a Conference
+      class NotifyRoleAssignedConference < Rectify::Command


Suggested change

class NotifyRoleAssignedConference < Rectify::Command

class NotifyRoleAssignedToConference < Rectify::Command

tramuntanal · 2020-04-23T14:35:28Z

...commands/decidim/participatory_processes/admin/notify_role_assigned_participatory_process.rb

+  module ParticipatoryProcesses
+    module Admin
+      # A command to notify users when a role is assigned for a participatory process
+      class NotifyRoleAssignedParticipatoryProcess < Rectify::Command


Suggested change

class NotifyRoleAssignedParticipatoryProcess < Rectify::Command

class NotifyRoleAssignedToParticipatoryProcess < Rectify::Command

tramuntanal · 2020-04-29T06:22:55Z

Thanks for the refactoring @anaghavl . It seems that "[CI] Admin / Tests" tests got stuck, I've re-run them.

mrcasals · 2020-04-29T07:18:59Z

@tramuntanal could we merge this anyway? Considering no permissions were modified, and no code from decidim-admin was modified, I think it would be safe... It's just that the CI is stuck again with locales 😞

* develop: (29 commits) Update Conversations design with decidim-design UI (decidim#6008) Add counter of active users to admin dashboard (decidim#5907) Show activity graphs on admin dashboard (decidim#6030) Update sassc gem version (decidim#6062) Fix generator Gemfile after puma upgrade (decidim#6060) New Crowdin translations (decidim#6059) Add Slovak as a new language (decidim#6039) Remove all tests for i18n PRs (decidim#6061) Update move up and down buttons after dragging questions when managing questionnaire (decidim#5947) Fix using Decidim as a provider for omniauth authentication (decidim#6042) Add redesign for responsive public profile navigation tabs (decidim#6032) Add versioning pages to initiatives (decidim#5935) Notify users when roles get assigned (decidim#5886) Improve the budget page and the project card (decidim#5809) New Crowdin translations (decidim#6050) New Crowdin translations (decidim#6046) Ignore jobs on locales branches (decidim#6047) Automatic task for deleting Meeting Inscription data (decidim#5989) New Crowdin translations (decidim#5877) Ignore builds on Crowdin PRs (decidim#6037) ...

mrcasals suggested changes Mar 23, 2020

View reviewed changes

agustibr suggested changes Mar 23, 2020

View reviewed changes

leio10 reviewed Mar 23, 2020

View reviewed changes

anaghavl requested review from agustibr, leio10 and mrcasals March 27, 2020 06:43

leio10 previously approved these changes Mar 27, 2020

View reviewed changes

anaghavl dismissed leio10’s stale review via 316a24f March 30, 2020 12:46

anaghavl force-pushed the 5733 branch from 7b7fa9b to 316a24f Compare March 30, 2020 12:46

mrcasals previously approved these changes Mar 31, 2020

View reviewed changes

andreslucena changed the title ~~5733 - Notify users when roles get assigned~~ Notify users when roles get assigned Mar 31, 2020

agustibr reviewed Apr 1, 2020

View reviewed changes

anaghavl dismissed mrcasals’s stale review via 543f016 April 3, 2020 11:31

anaghavl force-pushed the 5733 branch 2 times, most recently from 70e0e90 to e390914 Compare April 7, 2020 17:50

anaghavl force-pushed the 5733 branch from e390914 to cb1a491 Compare April 11, 2020 21:50

anaghavl requested review from agustibr, leio10 and mrcasals April 13, 2020 06:01

agustibr previously approved these changes Apr 13, 2020

View reviewed changes

leio10 previously approved these changes Apr 14, 2020

View reviewed changes

anaghavl dismissed stale reviews from leio10 and agustibr via 35d4a14 April 15, 2020 07:28

anaghavl force-pushed the 5733 branch from cb1a491 to 35d4a14 Compare April 15, 2020 07:28

mrcasals previously approved these changes Apr 15, 2020

View reviewed changes

agustibr previously approved these changes Apr 20, 2020

View reviewed changes

tramuntanal suggested changes Apr 23, 2020

View reviewed changes

anaghavl dismissed stale reviews from agustibr and mrcasals via 6fcd1cd April 27, 2020 18:56

anaghavl requested a review from tramuntanal April 27, 2020 18:57

anaghavl added 16 commits April 28, 2020 00:36

Notify on role for participatory process

a508387

Notify users on roles assigned

45e1d25

indentatiotion

64b5676

resolving comments

b554533

indentation

26a206b

adding changelog entry

d225483

fixing i18n error

7417c5c

Changing test files

3b36c32

changelog and indentation fix

d55cbce

changelog and indentation fix

dd026b6

Adding Email Notifications

0e5b814

normalizing en.yml files

de28265

test fix

b2c9c2d

Added tests for events

f2758f4

fixing changelog

fa65332

resolving comments

5b8389e

anaghavl force-pushed the 5733 branch from 6fcd1cd to 5b8389e Compare April 27, 2020 19:06

anaghavl added 2 commits April 28, 2020 00:56

renaming files

28e3289

comment fix

bdf6160

tramuntanal approved these changes Apr 30, 2020

View reviewed changes

tramuntanal merged commit 1f7f7a3 into develop Apr 30, 2020

tramuntanal deleted the 5733 branch April 30, 2020 06:55

	class AssemblyRoleAssignedEvent < Decidim::Events::SimpleEvent
	class RoleAssignedToAssemblyEvent < Decidim::Events::SimpleEvent

	class NotifyRoleAssignedAssembly < Rectify::Command
	class NotifyRoleAssignedToAssembly < Rectify::Command

	class NotifyRoleAssignedConference < Rectify::Command
	class NotifyRoleAssignedToConference < Rectify::Command

	class NotifyRoleAssignedParticipatoryProcess < Rectify::Command
	class NotifyRoleAssignedToParticipatoryProcess < Rectify::Command

Uh oh!

Conversation

anaghavl commented Mar 20, 2020 • edited by agustibr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

codecov bot commented Mar 20, 2020

Codecov Report

Uh oh!

anaghavl commented Mar 23, 2020

Uh oh!

mrcasals left a comment • edited by leio10 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agustibr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leio10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leio10 left a comment

Choose a reason for hiding this comment

Uh oh!

anaghavl commented Mar 31, 2020

Uh oh!

agustibr left a comment

Choose a reason for hiding this comment

Uh oh!

jesusdb commented Apr 1, 2020

Uh oh!

mrcasals commented Apr 14, 2020

Uh oh!

tramuntanal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tramuntanal commented Apr 29, 2020

Uh oh!

mrcasals commented Apr 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

anaghavl commented Mar 20, 2020 •

edited by agustibr

Loading

mrcasals left a comment •

edited by leio10

Loading

agustibr left a comment •

edited

Loading