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

Users can delete their account #1447

Merged
merged 9 commits into from
Jun 6, 2017
Merged
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
1 change: 0 additions & 1 deletion decidim-admin/app/helpers/decidim/admin/menu_helper.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# encoding: utf-8
# frozen_string_literal: true

module Decidim
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ const AddCommentFormWithMutation = graphql(addCommentMutation, {
__typename: "User",
name: ownProps.session.user.name,
avatarUrl: ownProps.session.user.avatarUrl,
deleted: false,
},
comments: [],
hasComments: false,
Expand Down
11 changes: 11 additions & 0 deletions decidim-comments/app/frontend/comments/comment.component.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ describe("<Comment />", () => {
expect(wrapper.find("a.author__name").text()).toEqual(comment.author.name);
});

describe("when the author's account has been deleted", () => {
beforeEach(() => {
comment.author.deleted = true;
});

it("should render 'Deleted user' inside a badge", () => {
const wrapper = shallow(<Comment comment={comment} session={session} />);
expect(wrapper.find("span.label.label--small.label--basic").text()).toEqual("Deleted user");
});
});

it("should render author's avatar as a image tag", () => {
const wrapper = shallow(<Comment comment={comment} session={session} />);
expect(wrapper.find("a.author__avatar img").prop("src")).toEqual(comment.author.avatarUrl);
Expand Down
6 changes: 5 additions & 1 deletion decidim-comments/app/frontend/comments/comment.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ class Comment extends React.Component<CommentProps, CommentState> {
<a className="author__avatar">
<img src={author.avatarUrl} alt="author-avatar" />
</a>
<a className="author__name">{author.name}</a>
{
author.deleted ?
<span className="label label--small label--basic">{I18n.t("components.comment.deleted_user")}</span> :
<a className="author__name">{author.name}</a>
}
<time dateTime={createdAt}>{formattedCreatedAt}</time>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ describe("<CommentThread />", () => {
const wrapper = shallow(<CommentThread comment={comment} session={session} />);
expect(wrapper.find("h6.comment-thread__title").text()).toContain(`Conversation with ${comment.author.name}`);
});

describe("when the author's account has been deleted", () => {
beforeEach(() => {
comment.author.deleted = true;
});

it("should render a h6 comment-thread__title with 'Deleted user'", () => {
const wrapper = shallow(<CommentThread comment={comment} session={session} />);
expect(wrapper.find("h6.comment-thread__title").text()).toContain("Conversation with Deleted user");
});
});
});

describe("should render a Comment", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ class CommentThread extends React.Component<CommentThreadProps, undefined> {
if (hasComments) {
return (
<h6 className="comment-thread__title">
{I18n.t("components.comment_thread.title", { authorName: author.name })}
{
author.deleted ?
I18n.t("components.comment_thread.title", { authorName: I18n.t("components.comment.deleted_user") }) :
I18n.t("components.comment_thread.title", { authorName: author.name })
}
</h6>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ fragment CommentData on Comment {
author {
name
avatarUrl
deleted
}
hasComments
acceptsNewComments
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const generateCommentsData = (num = 1) => {
author: {
name: name.findName(),
avatarUrl: image.imageUrl(),
deleted: false,
},
hasComments: false,
comments: [],
Expand Down
26 changes: 14 additions & 12 deletions decidim-comments/app/frontend/support/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ export interface GetCommentsQuery {
} | null,
} | null;
commentable: AddCommentFormCommentableFragment & {
// Wether the object can have new comments or not
// Whether the object can have new comments or not
acceptsNewComments: boolean,
// Wether the object comments have alignment or not
// Whether the object comments have alignment or not
commentsHaveAlignment: boolean,
// Wether the object comments have votes or not
// Whether the object comments have votes or not
commentsHaveVotes: boolean,
comments: Array< CommentThreadFragment & CommentFragment & CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment & {
// The Comment's unique ID
Expand All @@ -89,6 +89,14 @@ export interface AddCommentFormSessionFragment {
} >;
}

export interface CommentFragment extends CommentDataFragment, UpVoteButtonFragment, DownVoteButtonFragment {
comments: Array< CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment & {
comments: Array< CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment & {
comments: Array<CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment>,
} >,
} >;
}

export interface CommentDataFragment extends UpVoteButtonFragment, DownVoteButtonFragment {
// The Comment's unique ID
id: string;
Expand All @@ -106,10 +114,12 @@ export interface CommentDataFragment extends UpVoteButtonFragment, DownVoteButto
name: string,
// The author's avatar url
avatarUrl: string,
// Whether the author's account has been deleted or not
deleted: boolean,
};
// Check if the commentable has comments
hasComments: boolean;
// Wether the object can have new comments or not
// Whether the object can have new comments or not
acceptsNewComments: boolean;
// The comment's alignment. Can be 0 (neutral), 1 (in favor) or -1 (against)'
alignment: number | null;
Expand All @@ -122,14 +132,6 @@ export interface CommentThreadFragment extends CommentFragment, CommentDataFragm
hasComments: boolean;
}

export interface CommentFragment extends CommentDataFragment, UpVoteButtonFragment, DownVoteButtonFragment {
comments: Array< CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment & {
comments: Array< CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment & {
comments: Array<CommentDataFragment & UpVoteButtonFragment & DownVoteButtonFragment>,
} >,
} >;
}

export interface DownVoteButtonFragment {
// The Comment's unique ID
id: string;
Expand Down
1 change: 1 addition & 0 deletions decidim-comments/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ en:
alignment:
against: Against
in_favor: In favor
deleted_user: Deleted user
reply: Reply
report:
action: Report
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ module Decidim
module Comments
describe CommentNotificationMailer, type: :mailer do
let(:organization) { create(:organization) }
let(:participatory_process) { create(:participatory_process, organization: organization)}
let(:feature) { create(:feature, participatory_process: participatory_process)}
let(:participatory_process) { create(:participatory_process, organization: organization) }
let(:feature) { create(:feature, participatory_process: participatory_process) }
let(:commentable_author) { create(:user, organization: organization) }
let(:commentable) { create(:dummy_resource, author: commentable_author, feature: feature) }
let(:author) { create(:user, organization: organization) }
Expand Down
1 change: 1 addition & 0 deletions decidim-core/app/assets/javascripts/decidim.js.es6
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// = require decidim/history
// = require decidim/append_elements
// = require decidim/user_registrations
// = require decidim/account_form

/* globals svg4everybody */

Expand Down
27 changes: 27 additions & 0 deletions decidim-core/app/assets/javascripts/decidim/account_form.js.es6
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Since the delete account has a modal to confirm it we need to copy the content of the
* reason field to the hidden field in the form inside the modal.
Copy link
Contributor

Choose a reason for hiding this comment

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

What the hell? Is this a normal thing to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it isn't. The problem is the foundation reveal is rendered outside the real form and it is also a copy of the template marked as a popup. I know it's not a perfect solution but I needed this to solve the problem.

*/
$(() => {
const $deleteAccountForm = $('.delete-account');
const $deleteAccountModalForm = $('.delete-account-modal');

if ($deleteAccountForm.length > 0) {
const $openModalButton = $('.open-modal-button');
const $modal = $('#deleteConfirm');

$openModalButton.on('click', (event) => {
try {
const reasonValue = $deleteAccountForm.find('textarea#delete_account_delete_reason').val();
$deleteAccountModalForm.find('input#delete_account_delete_reason').val(reasonValue);
$modal.foundation('open');
} catch (error) {
console.error(error); // eslint-disable-line no-console
Copy link
Contributor

@mrcasals mrcasals Jun 6, 2017

Choose a reason for hiding this comment

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

Shouldn't we report this to Sentry instead of logging to the console

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have sentry configured for js errors but I think it's a good idea. I will do it in a separate PR

}

event.preventDefault();
event.stopPropagation();
return false;
});
}
});
47 changes: 47 additions & 0 deletions decidim-core/app/commands/decidim/destroy_account.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# frozen_string_literal: true

module Decidim
# This command destroys the user's account.
class DestroyAccount < Rectify::Command
# Destroy a user's account.
#
# user - The user to be updated.
# form - The form with the data.
def initialize(user, form)
@user = user
@form = form
end

def call
return broadcast(:invalid) unless @form.valid?

Decidim::User.transaction do
destroy_user_account!
destroy_user_identities
destroy_user_group_memberships
end

broadcast(:ok)
end

private

def destroy_user_account!
Copy link
Contributor

Choose a reason for hiding this comment

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

We should delete all the user's identities as well, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

And authorizations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrcasals Authorizations are anonymous and doesn't container user data so we are fine.

@user.name = ""
@user.email = ""
@user.delete_reason = @form.delete_reason
@user.deleted_at = Time.current
@user.skip_reconfirmation!
@user.remove_avatar!
@user.save!
end

def destroy_user_identities
@user.identities.destroy_all
end

def destroy_user_group_memberships
Decidim::UserGroupMembership.where(user: @user).destroy_all
end
end
end
23 changes: 23 additions & 0 deletions decidim-core/app/controllers/decidim/account_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,29 @@ def update
render action: :show
end

def delete
authorize! :delete, current_user
@form = form(DeleteAccountForm).from_model(current_user)
end

def destroy
authorize! :delete, current_user
@form = form(DeleteAccountForm).from_params(params)

DestroyAccount.call(current_user, @form) do
on(:ok) do
sign_out(current_user)
flash[:notice] = t("account.destroy.success", scope: "decidim")
end

on(:invalid) do
flash[:alert] = t("account.destroy.error", scope: "decidim")
end
end

redirect_to decidim.root_path
end

private

def authorizations
Expand Down
8 changes: 8 additions & 0 deletions decidim-core/app/forms/decidim/delete_account_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true

module Decidim
# The form object that handles the data behind deleting users account.
class DeleteAccountForm < Form
attribute :delete_reason, String
end
end
18 changes: 16 additions & 2 deletions decidim-core/app/models/decidim/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ class User < ApplicationRecord

ROLES = %w(admin moderator collaborator official).freeze

validates :organization, :name, presence: true
validates :organization, presence: true
validates :name, presence: true, unless: -> { deleted? }
validates :locale, inclusion: { in: I18n.available_locales.map(&:to_s) }, allow_blank: true
validates :tos_agreement, acceptance: true, allow_nil: false, on: :create
validates :avatar, file_size: { less_than_or_equal_to: MAXIMUM_AVATAR_FILE_SIZE }
validates :email, uniqueness: { scope: :organization }
validates :email, uniqueness: { scope: :organization }, unless: -> { deleted? }
validate :all_roles_are_valid
mount_uploader :avatar, Decidim::AvatarUploader

Expand Down Expand Up @@ -55,6 +56,11 @@ def name
super || I18n.t("decidim.anonymous_user")
end

# Check if the user account has been deleted or not
def deleted?
deleted_at.present?
end

# Check if the user exists with the given email and the current organization
#
# warden_conditions - A hash with the authentication conditions
Expand All @@ -69,6 +75,14 @@ def self.find_for_authentication(warden_conditions)
).first
end

protected

# Overrides devise email required validation.
# If the user has been deleted the email field is not required anymore.
def email_required?
!deleted?
end

private

def all_roles_are_valid
Expand Down
30 changes: 30 additions & 0 deletions decidim-core/app/views/decidim/account/delete.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<div class="row">
<div class="columns large-8 end">
<div class="callout alert">
<p><%= t('.alert') %></p>
</div>
<p><%= t('.explanation') %></p>
<%= decidim_form_for(@form, url: account_path, method: :delete, html: { class: "user-form delete-account" }) do |f| %>
<div>
<label>
<span class="user-form__label"><%= t('activemodel.attributes.account.delete_reason') %></span>
<%= f.text_area :delete_reason, rows: 2 %>
</label>
</div>
<input type="submit" class="button open-modal-button" value="<%= t('.confirm.title') %>" />
<% end %>
<div class="tiny reveal" id="deleteConfirm" data-reveal>
<%= decidim_form_for(@form, url: account_path, method: :delete, html: { class: "user-form delete-account-modal" }) do |f| %>
<%= f.hidden_field :delete_reason %>

<p><%= t('.confirm.question') %></p>

<input type="submit" class="button expanded" value="<%= t('.confirm.ok') %>" />

<button class="close-button" data-close aria-label="<%= t('.confirm.close') %>" type="button">
<span aria-hidden="true">&times;</span>
</button>
<% end %>
</div>
</div>
</div>
3 changes: 2 additions & 1 deletion decidim-core/app/views/layouts/decidim/user_profile.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@
<div class="columns medium-4 large-3">
<div class="side-panel">
<ul class="tabs vertical side-panel__tabs" id="user-settings-tabs">
<%= user_profile_tab t("account", scope: "layouts.decidim.user_profile"), account_path %>
<%= user_profile_tab t("account", scope: "layouts.decidim.user_profile"), account_path, aria_link_type: :exact %>
<%= user_profile_tab t("notifications_settings", scope: "layouts.decidim.user_profile"), notifications_settings_path %>
<% if available_authorization_handlers.any? %>
<%= user_profile_tab t("authorizations", scope: "layouts.decidim.user_profile"), authorizations_path %>
<% end %>
<% if user_groups.any? %>
<%= user_profile_tab t("user_groups", scope: "layouts.decidim.user_profile"), own_user_groups_path %>
<% end %>
<%= user_profile_tab t("delete_my_account", scope: "layouts.decidim.user_profile"), delete_account_path, aria_link_type: :exact %>
</ul>
</div>
</div>
Expand Down
Loading