Improve meeting invitations by rbngzlv · Pull Request #3632 · decidim/decidim

rbngzlv · 2018-06-11T09:18:18Z

🎩 What? Why?

Allow users to accept or reject invitations to meetings, and allow admins to see their status.

📌 Related Issues

Related to Improvement to meetings 1st Iteration #3233

📋 Subtasks

Add CHANGELOG entry
Add tests
Add feature to admin panel
Accept/Reject invite

📷 Screenshots

rbngzlv · 2018-06-11T13:33:55Z

Hi, I have doubts about the implementation of the part related to rejecting an invitation. Let's me explain the two possible cases:

1. An administrator invites an already registered user.

In this case we will always have the current_user, so no problem at all.

2. An administrator invites a non existing user.

In this case, the user is invited to sign up to the platform before accept the meeting invitation. So:

If the user accepts and completes the sign up form, it is redirected to the "accept invitation" URL (as it is doing now).
If the user wants to refuse the invitation, we need to "reject the invitation" but don't touch the invitation to sign up, because maybe the user has also been invited to another meeting or by a friend, right? And also, we will need a token to identify the meeting invitation that is being refused, because we will not have the current_user.

/cc @decidim/product @decidim/lot-core

josepjaume · 2018-06-11T13:35:17Z

Hi @rbngzlv - @mrcasals has recently been working on invitations so maybe he can provide some feedback.

mrcasals · 2018-06-11T13:46:22Z

I don't have much insight, I delegates the logic to the InviteUser command, which handled my use cases.

If the user wants to refuse the invitation, we need to "reject the invitation" but don't touch the invitation to sign up, because maybe the user has also been invited to another meeting or by a friend, right? And also, we will need a token to identify the meeting invitation that is being refused, because we will not have the current_user.

We could make a list of invitations, so after a sign up we redirect the user to this list (if there's any invitations, of course). From there, the user can accept or reject those invitations (one button for each action). This would work for already existing users too. This way we don't need to send tokens through the invitation email.

Does this sound good to you @rbngzlv? Would this fix your problem? If this works for you please ensure @decidim/product is OK with this 😄

rbngzlv · 2018-06-11T15:08:36Z

@mrcasals the idea of a central place to manage the invitations sounds good to me, but I think that it is out of the scope of this PR and also doesn't solve the "problem" with the unregistered users refusing invitations, because, an unregistered user will not be able to access that page.

mrcasals · 2018-06-12T07:13:42Z

@rbngzlv when we invite a user to the platform we create the User in the DB, but we differentiate those users that have accepted the invitation from those that haven't. We could create MeetingInvitation model related to a user and a meeting. If the user never accepts the invitation to the platform, we'll have to leave the meeting invitation row around. We can make it expire though, for example:

if you haven't accepted a meeting invitation in 1 month, then it is deleted
if the meeting starts, all invitations to that meeting are deleted

the idea of a central place to manage the invitations sounds good to me, but I think that it is out of the scope of this PR

I don't think that's out of the scope of the PR, but I'll leave that to @decidim/product. A list of meeting invitations is the best I can come up with, if you have another layout/workflow in mind then go ahead 😄

isaacmg410 · 2018-06-25T11:13:53Z

@rbngzlv @decidim/lot-core ready to check, no?

mrcasals · 2018-07-03T13:48:49Z

@isaacmg410 you'll need to rebase/update your branch with master so that tests can run!

isaacmg410 · 2018-07-03T13:57:37Z

@rbngzlv check @mrcasals comment

rbngzlv · 2018-07-03T14:43:27Z

@mrcasals master merged.

rbngzlv · 2018-07-05T08:06:04Z

Hi @decidim/lot-core, it is the fifth time that we merge master to resolve conflicts, can you review this PR please?

/cc @decidim/lot-mods

arnaumonty · 2018-07-09T09:42:38Z

@decidim/lot-core @mrcasals can you review this, please?

mrcasals · 2018-07-09T09:52:41Z

decidim-meetings/app/views/devise/mailer/join_meeting.html.erb

  <%= t("decidim.meetings.admin.invite_join_meeting_mailer.invite.invited_you_to_join_a_meeting", invited_by: @resource.invited_by.name, application: @resource.organization.name) %>
 </p>

+<p><%= link_to t("devise.mailer.invitation_instructions.decline"), accept_invitation_url(@resource, invitation_token: @token, invite_redirect: Decidim::EngineRouter.main_proxy(@opts[:meeting].component).decline_invitation_meeting_registration_path(meeting_id: @opts[:meeting], participatory_space_id: @opts[:meeting].component.participatory_space), host: @resource.organization.host) %></p>


What does Decidim::EngineRouter.main_proxy(@opts[:meeting].component) do? Have you tried with decidim_meetings.decline_invitation_meeting_registration_path?

I think that we can't access to decicim_meetings because this mailer is executed inside devise.

mrcasals · 2018-07-09T09:52:50Z

decidim-meetings/app/views/devise/mailer/join_meeting.text.erb

 <%= t("decidim.meetings.admin.invite_join_meeting_mailer.invite.invited_you_to_join_a_meeting", invited_by: @resource.invited_by.name, application: @resource.organization.name) %>

+<%= t("devise.mailer.invitation_instructions.decline") %>:
+<% accept_invitation_url(@resource, invitation_token: @token, invite_redirect: Decidim::EngineRouter.main_proxy(@opts[:meeting].component).decline_invitation_meeting_registration_path(meeting_id: @opts[:meeting], participatory_space_id: @opts[:meeting].component.participatory_space), host: @resource.organization.host) %>


I think that we can't access to decicim_mettings because this mailer is executed inside devise.

mrcasals · 2018-07-09T09:54:55Z

decidim-meetings/spec/models/meeting_spec.rb


    it "has an association with one agenda" do
-      subject.agenda = build(:agenda)
+      subject.agenda = build_stubbed(:agenda)


Oh, I didn't know about this build_stubbed method! 👏

mrcasals · 2018-07-09T09:58:05Z

Code looks good, build was green. Conflicts were only on the CHANGELOG file.

@rbngzlv sorry for the delay reviewing this! Please, if this happens again, ping me privately on Gitter and I'll check it ASAP. GitHub notifications are far from great and it's not the first time some important things slip my filters 😞

Also, even though the PR is merged, can you check this comment? #3632

Thanks!

oriolgual · 2018-07-09T09:48:50Z

decidim-meetings/app/commands/decidim/meetings/decline_invitation.rb

+      end
+
+      def can_decline_invitation?
+        meeting.registrations_enabled? && invitation.present?


What happens if the registrations are disabled after I get the invite? Does it appear as pending?

Yes, it appears as pending. If the user don't respond the invitation before the registrations are closed, he can't either accept it or reject it, her time to interact is over. I think that for admin is important to know that the user doesn't respond in time.

oriolgual · 2018-07-09T09:50:28Z

decidim-meetings/app/controllers/decidim/meetings/admin/invites_controller.rb

      #
      class InvitesController < Admin::ApplicationController
+        def index
+          enforce_permission_to :read_invites, :meeting, meeting: meeting


How come we have a single meeting if we are at the index?

This action is listing the invites for a single meeting. Is a screen in the admin panel.

Oh, it's listing invites, not meetings. Sorry, my bad 🤦‍♂️

oriolgual · 2018-07-09T09:51:38Z

decidim-meetings/app/models/decidim/meetings/invite.rb

+        Decidim::Meetings::AdminLog::InvitePresenter
+      end
+
+      def self.user_collection(user)


Where is this used?

This is needed for the data portability export.

oriolgual · 2018-07-09T09:53:21Z

decidim-meetings/app/views/decidim/meetings/admin/invites/index.html.erb

+          <% end %>
+        </tbody>
+      </table>
+      <%= paginate @invites, theme: "decidim" %>


Can you use decidim_paginate?

I can't localize an admin view using decidim_paginate, I need to include the Decidim::PaginateHelper in Decidim::Admin::ApplicationHelper, is this helper new? Do you want this?

Oh, I thought we were using it everywhere, don't worry then.

oriolgual · 2018-07-09T09:55:11Z

decidim-meetings/app/queries/decidim/meetings/admin/invites.rb

+        def filter_by_search(invites)
+          return invites if @query.blank?
+          invites.joins(:user).where(
+            User.arel_table[:name].lower.matches("%#{@query}%").or(User.arel_table[:email].lower.matches("%#{@query}%"))


We should check that the users are from the same organization

Not sure about this, the query is loading only the users related to the invites being filtered, right?

SELECT "decidim_meetings_invites".* FROM "decidim_meetings_invites" INNER JOIN "decidim_users" ON "decidim_users"."id" = "decidim_meetings_invites"."decidim_user_id" WHERE "decidim_meetings_invites"."decidim_meeting_id" = $1 AND (LOWER("decidim_users"."name") ILIKE '%ab%' OR LOWER("decidim_users"."email") ILIKE '%ab%') LIMIT $2 OFFSET $3

Oh, you're right, no need for that!

oriolgual · 2018-07-09T10:00:22Z

It was merged while I was reviewing 😞

@rbngzlv please create a new PR with the fixes.

mrcasals · 2018-07-09T10:00:45Z

Whoops, sorry :(

rbngzlv · 2018-07-09T10:02:26Z

Thank you both for the review. I'll check all comments, discuss here if needed, and create a new PR, don't worry.

rbngzlv · 2018-07-09T14:46:30Z

Hi @oriolgual and @mrcasals can you check my comments, please?

oriolgual · 2018-07-11T08:28:10Z

Done @rbngzlv

rbngzlv added 4 commits June 11, 2018 10:39

Add invites to admin panel

327feb6

Allow to search and filter invites in admin

17f23d3

Add specs for admin part

a02c583

Normalize comments bundle

b49c593

rbngzlv added the status: WIP label Jun 11, 2018

rbngzlv self-assigned this Jun 11, 2018

xabier mentioned this pull request Jun 11, 2018

Improvement to meetings 1st Iteration #3233

Closed

6 tasks

rbngzlv force-pushed the feature/improve_meeting_invitations branch from 146888f to 8cebc8d Compare June 14, 2018 15:34

Allow user to accept or decline invitation

0a693d9

rbngzlv force-pushed the feature/improve_meeting_invitations branch from 8cebc8d to 0a693d9 Compare June 14, 2018 21:22

rbngzlv added 4 commits June 15, 2018 00:40

Show an error if user has already been invited

d764cfc

Merge branch 'master' into feature/improve_meeting_invitations

6bbcc5a

Add change log entry

d84f59b

Merge branch 'master' into feature/improve_meeting_invitations

5460fa4

rbngzlv added in-review and removed status: WIP labels Jun 22, 2018

Merge branch 'master' into feature/improve_meeting_invitations

28c0c5f

ghost added the status: WIP label Jun 22, 2018

isaacmg410 removed the status: WIP label Jun 25, 2018

Merge branch 'master' into feature/improve_meeting_invitations

f35a5a9

ghost added the status: WIP label Jul 1, 2018

Merge branch 'master' into feature/improve_meeting_invitations

350d755

rbngzlv removed the status: WIP label Jul 3, 2018

Merge branch 'master' into feature/improve_meeting_invitations

2bd30c9

ghost added the status: WIP label Jul 5, 2018

rbngzlv removed the status: WIP label Jul 5, 2018

mrcasals previously approved these changes Jul 9, 2018

View reviewed changes

Merge branch 'master' into feature/improve_meeting_invitations

682937f

mrcasals dismissed their stale review via 682937f July 9, 2018 09:55

ghost assigned mrcasals Jul 9, 2018

ghost added the status: WIP label Jul 9, 2018

mrcasals merged commit eca456a into master Jul 9, 2018

mrcasals deleted the feature/improve_meeting_invitations branch July 9, 2018 09:56

ghost removed the status: WIP label Jul 9, 2018

oriolgual reviewed Jul 9, 2018

View reviewed changes

Uh oh!

Conversation

rbngzlv commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots

Uh oh!

rbngzlv commented Jun 11, 2018

Uh oh!

josepjaume commented Jun 11, 2018

Uh oh!

mrcasals commented Jun 11, 2018

Uh oh!

rbngzlv commented Jun 11, 2018

Uh oh!

mrcasals commented Jun 12, 2018

Uh oh!

isaacmg410 commented Jun 25, 2018

Uh oh!

mrcasals commented Jul 3, 2018

Uh oh!

isaacmg410 commented Jul 3, 2018

Uh oh!

rbngzlv commented Jul 3, 2018

Uh oh!

rbngzlv commented Jul 5, 2018

Uh oh!

arnaumonty commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbngzlv Jul 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

rbngzlv Jul 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbngzlv Jul 9, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

rbngzlv Jul 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

oriolgual commented Jul 9, 2018

Uh oh!

mrcasals commented Jul 9, 2018

rbngzlv commented Jun 11, 2018 •

edited

Loading

rbngzlv Jul 9, 2018 •

edited

Loading

rbngzlv Jul 9, 2018 •

edited

Loading

rbngzlv Jul 9, 2018 •

edited

Loading

rbngzlv Jul 9, 2018 •

edited

Loading