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 5 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

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
18 changes: 10 additions & 8 deletions decidim-comments/app/frontend/support/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,6 +114,8 @@ export interface CommentDataFragment extends UpVoteButtonFragment, DownVoteButto
name: string,
// The author's avatar url
avatarUrl: string,
// Wheter the author's account has been deleted or not
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Wheter/Whether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is auto-generated so I made the change but didn't regenerate the schema 😄

deleted: boolean,
};
// Check if the commentable has comments
hasComments: boolean;
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
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;
});
}
});
32 changes: 32 additions & 0 deletions decidim-core/app/commands/decidim/destroy_account.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# 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?

destroy_user_account!
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.email = "deleted-user-#{SecureRandom.uuid}@example.org"
Copy link
Contributor

Choose a reason for hiding this comment

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

email can be empty when the user is deleted. There's a special index for that (not sure about the model's validation) but I think is way better to deal it this way.

Copy link
Contributor Author

@beagleknight beagleknight Jun 6, 2017

Choose a reason for hiding this comment

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

And erase their name

@user.delete_reason = @form.delete_reason
@user.deleted_at = Time.current
@user.skip_reconfirmation!
@user.save!
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
5 changes: 5 additions & 0 deletions decidim-core/app/models/decidim/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,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 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, placeholder: t('activemodel.placeholders.account.delete_reason') %>
</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
17 changes: 17 additions & 0 deletions decidim-core/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
en:
activemodel:
attributes:
account:
delete_reason: Reason
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change this to: "Reason to delete your account"

report:
details: Additional comments
user:
Expand All @@ -13,6 +15,9 @@ en:
user_group_document_number: Organization document number
user_group_name: Organization name
user_group_phone: Organization phone
placeholders:
account:
delete_reason: Reason
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need a placeholder here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the template but I think you are right.

activerecord:
attributes:
decidim/user:
Expand All @@ -33,6 +38,17 @@ en:
image_too_big: The image is too big
decidim:
account:
delete:
alert: This action cannot be undone. If you delete your account you won't be able to log in.
confirm:
close: Close window
ok: Yes, I want to delete my account
question: Are you sure you want to delete your account?
title: Delete my account
explanation: Please, fill in the reason you want to delete your account (optional).
destroy:
error: There's been an error deleting your account.
success: Your account was deleted successfully.
show:
change_password: Change password
update_account: Update account
Expand Down Expand Up @@ -322,6 +338,7 @@ en:
user_profile:
account: Account
authorizations: Authorizations
delete_my_account: Delete my account
notifications_settings: Notifications settings
title: User settings
user_groups: Organizations
Expand Down
6 changes: 5 additions & 1 deletion decidim-core/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@
get :first_login
end
end
resource :account, only: [:show, :update], controller: "account"
resource :account, only: [:show, :update, :destroy], controller: "account" do
member do
get :delete
end
end
resource :notifications_settings, only: [:show, :update], controller: "notifications_settings"
resources :own_user_groups, only: [:index]
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddDeletedFieldsToUsers < ActiveRecord::Migration[5.0]
def change
add_column :decidim_users, :delete_reason, :text
add_column :decidim_users, :deleted_at, :datetime
end
end
3 changes: 3 additions & 0 deletions decidim-core/lib/decidim/core/api/author_interface.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ module Decidim
description "An author"

field :name, !types.String, "The author's name"

field :avatarUrl, !types.String, "The author's avatar url"

field :deleted, !types.Boolean, "Wheter the author's account has been deleted or not"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Wheter/Whether/

end
end
4 changes: 4 additions & 0 deletions decidim-core/lib/decidim/core/api/user_group_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,9 @@ module Decidim
field :avatarUrl, !types.String, "The user's avatar url" do
resolve ->(obj, _args, _ctx) { obj.avatar.url }
end

field :deleted, !types.Boolean, "Whether the user group's has been deleted or not" do
resolve ->(_obj, _args, _ctx) { false }
end
end
end
Loading