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

🦋Update provider account Invitations index page #3923

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions app/controllers/provider/admin/account/invitations_controller.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# frozen_string_literal: true

class Provider::Admin::Account::InvitationsController < Provider::Admin::Account::BaseController
before_action :authorize_multiple_users
before_action :set_resource
Expand All @@ -6,12 +8,8 @@ class Provider::Admin::Account::InvitationsController < Provider::Admin::Account
inherit_resources
belongs_to :account


create! do |success, failure|
success.html do
redirect_to provider_admin_account_invitations_path,
notice: 'Invitation will be sent soon.'
end
success.html { redirect_to provider_admin_account_invitations_path, notice: t('.success') }
end

destroy! do |success, failure|
Expand All @@ -23,11 +21,8 @@ def resend
@invitation.resend

respond_to do |format|
format.html do
flash[:success] = 'Invitation will be resent soon.'
redirect_to provider_admin_account_invitations_path
end
format.xml { head :ok }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is not used anymore?

Copy link
Contributor Author

@josemigallas josemigallas Oct 16, 2024

Choose a reason for hiding this comment

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

No, not really but it was added 10+ years ago and I think XML responses are only used by API endpoints.

There is nothing about it in 3scale API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPDATE: After some digging I can't really justify removing this response so I will restore it and add a comment for posterity.

format.html { redirect_to provider_admin_account_invitations_path, notice: t('.success') }
format.xml { head :ok } # TODO: figure out if this is still used or it needs to be cleaned up
end
end

Expand All @@ -38,7 +33,7 @@ def authorize_multiple_users
end

def collection
@invitations ||= end_of_association_chain.paginate(:page => params[:page])
@collection ||= end_of_association_chain
end

def set_resource
Expand Down
3 changes: 2 additions & 1 deletion app/helpers/buttons_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ def dropdown_link(*args)
end

def pf_link_to(label, url, options = {})
options[:class] = join_dom_classes('pf-c-button pf-m-link', options[:class])
variant = options.delete(:variant) || :link
options[:class] = join_dom_classes("pf-c-button pf-m-#{variant}", options[:class])
link_to label, url, options
end

Expand Down
8 changes: 3 additions & 5 deletions app/helpers/invitations_helper.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# frozen_string_literal: true

# TODO: remove and use presenter
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are only two small methods left here, why not move them to the presenter directly instead of leaving a TODO?

module InvitationsHelper
def invitation_sent_date(invitation)
invitation.sent_at&.to_s(:long) || 'Not sent yet'
Expand All @@ -10,9 +13,4 @@ def invitation_status(invitation)
"no"
end
end

def button_to_resend_buyer_invitation(invitation)
fancy_link_to('Resend', resend_provider_admin_account_invitation_path(invitation.account,invitation),
{ :id => "resend-invitation-#{invitation.id}", :method => :put })
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# frozen_string_literal: true

class Provider::Admin::Account::InvitationsIndexPresenter
include System::UrlHelpers.system_url_helpers
include InvitationsHelper

alias status invitation_status
alias sent_date invitation_sent_date

def initialize(invitations, user, params)
@ability = Ability.new(user)
pagination_params = { page: params[:page] || 1, per_page: params[:per_page] || 20 }
sorting_params = "#{params[:sort].presence || 'sent_at'} #{params[:direction].presence || 'desc'}"
@invitations = invitations.paginate(pagination_params)
.reorder(sorting_params)
end

attr_reader :invitations

def empty_state?
@invitations.total_entries.zero?
end

def can_send_invitations?
@ability.can?(:create, Invitation) && @ability.can?(:see, :multiple_users)
end

# def sent_date(invitation)
# invitation.sent_at&.to_s(:long) || 'Not sent yet'
# end

# def status(invitation)
# invitation_status(invitation)
# # if invitation.accepted?
# # "yes, on #{invitation.accepted_at.to_s(:short)}"
# # else
# # 'no'
# # end
# end
Comment on lines +28 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

Better don't push commented code IMO


def can_manage_invitation?(invitation)
@can_manage_invitation ||= @ability.can?(:manage, invitation)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong, after the first call, it will cache the value of @ability.can?(:manage, invitation) and return it on subsequent calls even when called with different invitation

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would actually work in this case, because for the provider users the abilities are for the whole class, there is no condition that would make it different for different invitations - it's either allowed for all, or not allowed for all.

But I agree that still it's not right 😬

end

def toolbar_props
{
totalEntries: @invitations.total_entries,
actions: [{
variant: :primary,
label: I18n.t('provider.admin.account.invitations.index.send_invitation_title'),
href: new_provider_admin_account_invitation_path
}]
}
end

end
47 changes: 23 additions & 24 deletions app/views/provider/admin/account/invitations/index.html.erb
Original file line number Diff line number Diff line change
@@ -1,52 +1,53 @@
<% content_for :page_header_title, 'Invitations' %>

<% content_for :javascripts do %>
<%# HACK: temporary reuse css of existing pack "table_toolbar". Once toolbar is implemented,
import the whole pack with "javascript_packs_with_chunks_tag" %>
<%= stylesheet_packs_chunks_tag 'table_toolbar' %>
<%= javascript_packs_with_chunks_tag 'table_toolbar' %>
<% end %>

<% if @invitations.total_entries.zero? %>
<% presenter = Provider::Admin::Account::InvitationsIndexPresenter.new(collection, current_user, params) %>

<% if presenter.empty_state? %>
<%= render partial: 'shared/empty_state', locals: { icon: 'plus-circle',
title: t('.empty_state_title'),
body: t('.empty_state_body'),
primary: { title: t('.empty_state_primary_title'),
href: new_provider_admin_account_invitation_path } } %>
<% else %>
<table class="pf-c-table pf-m-grid-lg" role="grid" aria-label="Invitations table" id="invitations">
<table class="pf-c-table pf-m-grid-lg" role="grid" aria-label="Invitations table" data-toolbar-props="<%= presenter.toolbar_props.to_json %>">
<thead>
<tr role="row">
<th role="columnheader" scope="col">Recipient</th>
<th role="columnheader" scope="col">Sent</th>
<th role="columnheader" scope="col">Accepted?</th>
<th role="columnheader" scope="col" class="pf-c-table__action pf-m-fit-content">
<% if can?(:create, Invitation) and can?(:see, :multiple_users) %>
<%= link_to_unless_current 'Invite a New Team Member', new_provider_admin_account_invitation_path, :class => 'action add' %>
<% end %>
</th>
<%= th_sortable 'sent_at', 'Sent' %>
<%= th_sortable 'accepted_at', 'Accepted?' %>
<td></td>
</tr>
</thead>

<tbody role="rowgroup">
<% @invitations.each do |invitation| %>
<% presenter.invitations.each do |invitation| %>
<tr role="row" id="<%= dom_id(invitation) %>">
<td role="cell" data-label="Recipient"><%= h invitation.email %></td>
<td role="cell" data-label="Sent"><%= invitation_sent_date(invitation) %></td>
<td role="cell" data-label="Accepted?"><%= invitation_status(invitation) %></td>
<td role="cell" data-label="Sent"><%= presenter.sent_date(invitation) %></td>
<td role="cell" data-label="Accepted?"><%= presenter.status(invitation) %></td>
<td role="cell" class="pf-c-table__action">
<div class="pf-c-overflow-menu">
<div class="pf-c-overflow-menu__content">
<div class="pf-c-overflow-menu__group pf-m-button-group">
<% if can? :manage, invitation %>
<% if presenter.can_manage_invitation?(invitation) %>
<div class="pf-c-overflow-menu__item">
<% unless invitation.accepted? -%>
<%= fancy_link_to("Resend", resend_provider_admin_account_invitation_path(invitation), {:method => :put, :class => 'refresh', "data-id" => invitation.id}) %>
<% end -%>
<%= pf_link_to 'Resend', resend_provider_admin_account_invitation_path(invitation),
method: :put,
variant: invitation.accepted? ? :disabled : :primary,
class: 'refresh',
data: { id: invitation.id } %>
</div>
<% end -%>
<% if can? :manage, invitation %>
<% end %>
<% if presenter.can_manage_invitation?(invitation) %>
<div class="pf-c-overflow-menu__item">
<%= delete_link_for provider_admin_account_invitation_path(invitation), data: { confirm: 'Are you sure you want to delete this invitation?' } %>
<%= pf_link_to 'Delete', provider_admin_account_invitation_path(invitation),
data: { confirm: t('.delete_confirm') },
variant: :danger,
method: :delete %>
</div>
<% end %>
</div>
Expand All @@ -57,6 +58,4 @@
<% end %>
</tbody>
</table>

<%= will_paginate @invitations %>
<% end %>
8 changes: 7 additions & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -404,10 +404,16 @@ en:
admin:
account:
invitations:
create:
success: Invitation will be sent soon.
index:
delete_confirm: Are you sure you want to delete this invitation?
empty_state_title: No invitations
empty_state_body: To add new members to your team, send them an invitation to join.
empty_state_primary_title: Invite a team member
empty_state_primary_title: Invite a new team member
send_invitation_title: Invite a new team member
resend:
success: Invitation will be resent soon.
users:
form:
submit_button_label: 'Update User'
Expand Down
98 changes: 0 additions & 98 deletions features/old/accounts/invitations.feature
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,6 @@ Feature: Invitations
And provider "foo.3scale.localhost" has multiple applications enabled
And provider "foo.3scale.localhost" has "multiple_users" switch allowed

@javascript
Scenario: When switch is denied as provider
Given current domain is the admin domain of provider "foo.3scale.localhost"
And provider "foo.3scale.localhost" has "multiple_users" switch denied
When I log in as provider "foo.3scale.localhost"
And I go to the provider users page
Then I should not see "Invite a New Team Member"

When I request the url of the provider new invitation page then I should see an exception

Scenario: When switch is denied as buyer
Given a buyer "apininja" signed up to provider "foo.3scale.localhost"
And provider "foo.3scale.localhost" has "multiple_users" switch denied
Expand All @@ -37,91 +27,3 @@ Feature: Invitations
And I fill in "Send invitation to" with "alice@foo.3scale.localhost"
And I press "Invite User"
Then invitation from account "apininja" should be sent to "alice@foo.3scale.localhost"

@javascript
Scenario: Attempt to send invitation to an email of already existing user
Given an user "alice" of account "foo.3scale.localhost"
And user "alice" has email "alice@foo.3scale.localhost"
And current domain is the admin domain of provider "foo.3scale.localhost"
When I log in as provider "foo.3scale.localhost"
And I go to the provider new invitation page
And I fill in "Send invitation to" with "alice@foo.3scale.localhost"
And I press "Send"
Then I should see error "has been taken by another user" for field "Send invitation to"
And no invitation should be sent to "alice@foo.3scale.localhost"

@javascript
Scenario: Invitation to an email of already existing pending invitation
Given an invitation from account "foo.3scale.localhost" sent to "alice@foo.3scale.localhost"
And a clear email queue
And current domain is the admin domain of provider "foo.3scale.localhost"
When I log in as provider "foo.3scale.localhost"
And I go to the provider new invitation page
And I fill in "Send invitation to" with "alice@foo.3scale.localhost"
And I press "Send"
Then I should see "This invitation has already been sent."

@javascript
Scenario: Deleted user from invitation with changed email
Given an invitation from account "foo.3scale.localhost" sent to "ubuntu@foo.3scale.localhost"
When I follow the link to signup provider "foo.3scale.localhost" in the invitation sent to "ubuntu@foo.3scale.localhost"
And I fill in the invitation signup with email "gentoo@foo.3scale.localhost"
When I log in as provider "foo.3scale.localhost"
And I go to the provider users page
Then I should see "gentoo@foo.3scale.localhost"
And I should not see "ubuntu@foo.3scale.localhost"
Then I press "Delete" for user "gentoo@foo.3scale.localhost" and confirm the dialog
And I go to the provider sent invitations page
Then I should not see "ubuntu@foo.3scale.localhost"

@javascript
Scenario: Accepting an invitation
Given an invitation from account "foo.3scale.localhost" sent to "alice@foo.3scale.localhost"
And all the rolling updates features are off
And current domain is the admin domain of provider "foo.3scale.localhost"
When I follow the link to signup in the invitation sent to "alice@foo.3scale.localhost"
And I fill in the invitation signup as "alice"
And current domain is the admin domain of provider "foo.3scale.localhost"
When I try to log in as provider "alice"
Then I should be logged in as "alice"
And follow "API"
And I should see "Analytics"

@javascript
Scenario: Managing sent invitations
Given the following invitations from account "foo.3scale.localhost" exist:
| Email | State |
| alice@foo.3scale.localhost | pending |
| bob@foo.3scale.localhost | accepted |
And current domain is the admin domain of provider "foo.3scale.localhost"
When I log in as provider "foo.3scale.localhost"
And I go to the provider users page
And I follow "Invitations"
Then I should see "Invitations" in a header
And I should see pending invitation for "alice@foo.3scale.localhost"
And I should see accepted invitation for "bob@foo.3scale.localhost"

@javascript
Scenario: Deleting an invitation
Given an invitation from account "foo.3scale.localhost" sent to "alice@foo.3scale.localhost"
And current domain is the admin domain of provider "foo.3scale.localhost"
When I log in as provider "foo.3scale.localhost"
And I go to the provider sent invitations page
And I press "Delete" for an invitation from account "foo.3scale.localhost" for "alice@foo.3scale.localhost" and confirm the dialog
Then I should not see invitation for "alice@foo.3scale.localhost"

@javascript
Scenario: Managing sent invitations disabled when multiple_users switch denied
Given provider "foo.3scale.localhost" has "multiple_users" switch denied
And current domain is the admin domain of provider "foo.3scale.localhost"
When I log in as provider "foo.3scale.localhost"
And I go to the provider users page
Then I should not see "Invitations"

@security @allow-rescue @javascript
Scenario: Only admins can send invitations
Given an active user "alice" of account "foo.3scale.localhost"
And current domain is the admin domain of provider "foo.3scale.localhost"
When I log in as provider "alice"
And I go to the provider new invitation page
Then I should be denied the access
12 changes: 7 additions & 5 deletions features/old/buyers/invitations/for_admins.feature
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
@javascript
Feature: Invitations on partner accounts for admins
In order to allow provider account admins to administer their partner accounts
As an admin I want to manage the invitations of users to the partner accounts
Feature: Buyer Account Invitations

TODO: update as part of THREESCALE-9876 in the same way as features/provider/admin/account/invitations.feature

Background:
Given the default product of provider "master" has name "Master API"
Expand Down Expand Up @@ -41,12 +41,14 @@ Feature: Invitations on partner accounts for admins
Given an invitation sent to "alice@lolcats.com" to join account "lol cats" was accepted
And an invitation sent to "bob@lolcats.com" to join account "lol cats"
When I navigate to the page of the invitations of the partner "lol cats"
Then I should see accepted invitation for "alice@lolcats.com"
And I should see pending invitation for "bob@lolcats.com"
Then the table should contain an accepted invitation from "alice@lolcats.com"
And the table should contain a pending invitation from "bob@lolcats.com"

Scenario: Destroying invitations
Given an invitation sent to "alice@lolcats.com" to join account "lol cats"
When I navigate to the page of the invitations of the partner "lol cats"
# And select action "Delete" of "alice@example.org"
# And confirm the dialog
And I press "Delete" for an invitation from account "lol cats" for "alice@lolcats.com" and confirm the dialog
Then I should not see invitation for "alice@lolcats.com"

Expand Down
15 changes: 0 additions & 15 deletions features/old/providers/invitations.feature

This file was deleted.

Loading