Delete a organization + organization policy #368

markets · 2018-05-26T18:20:21Z

Closes #359
Closes #424

We need a way to remove all Organitzation activity through active admin panel

Changes:

add some missing dependent: :destroy to properly delete all organization's activity and traces when destroying an instance
✂️ OrganizationsController#destroy endpoint from the public part, so organizations should be removed from the admin section
Introduced OrganizationPolicy (logic partially extracted from current visibility of links in views, but please, let me know what do you think or we should change/tune something 👀):
- index public
- show public
- new/create superadmins
- edit/update superadmins or organization admin
Updated Pundit to latest release (0.3.0 => 1.1.0 => 2.0.0)
Paginate organizations list
Moar tests!

sauloperez · 2018-05-28T09:17:11Z

app/policies/organization_policy.rb

+  end
+
+  def show?
+    user&.superadmin? || user&.active?(organization)


Just for me to understand, when do we have a user here and when not?

Pundit uses the following to setup the user variable:

timeoverflow/app/controllers/application_controller.rb

Lines 73 to 79 in b31466a

def current_member

@current_member ||= current_user.as_member_of(current_organization) if current_user

end

def pundit_user

current_member

end

Sometimes there is no current_user, so we should check its presence at least in controllers (like OrganizationsController) that don't use before_action :authenticate_user!.

sauloperez · 2018-05-28T09:17:29Z

app/views/organizations/index.html.erb

@@ -27,20 +27,16 @@
        <td><%= link_to org.name, org %></td>
        <td><%= org.users.count %></td>
        <td class="hover-actions">
-          <% if current_user.admins?(org) %>
+          <% if current_user&.admins?(org) %>


I have the same question here

Similar to previous comment, this controller doesn't require authentication at this point, so now you can visit the /organizations page without a crash.

sauloperez · 2018-05-28T09:20:17Z

spec/controllers/organizations_controller_spec.rb

+
+        get 'show', id: organization.id
+        expect(response.body).to include(
+          "<a href=\"/transfers/new?destination_account_id=#{organization.account.id}&amp;id=#{organization.id}\">"


IMO views specs are better suited for this. We started doing it like this and when we started feeling the pain we switched to views specs.

Totally agree! view specs are a better option to test those kind of things

markets · 2018-05-28T20:29:23Z

@sauloperez @sseerrggii ready for review 👀

Let me know, especially, what do you think about this organization policy:

index is public
show superadmins or active members
new/create superadmins
edit/update superadmins or organization admin
destroy ✂️ only allowed from /admin section

sauloperez · 2018-05-29T06:41:01Z

app/views/organizations/index.html.erb

          <% end %>
        </td>
      </tr>
    <% end %>
  </tbody>
 </table>
+
+<%= paginate @organizations %>


are we showing all organizations now??

unfortunately yes, without pagination 🙊

sseerrggii · 2018-05-29T07:08:06Z

show is public now and it's good because you can find organizations contact information (some day we want to add a map in TimeOverflow homepage)

https://www.timeoverflow.org/organizations/3

Note: in this view when you are a member of this organitzation you can see also the account (balance, transfers) of the organization

markets · 2018-05-29T07:54:21Z

Ok @sseerrggii I'll change policy for show action. Thanks!

markets · 2018-05-29T19:28:19Z

@sseerrggii @sauloperez changes done and PR description updated.

enricostano · 2018-06-07T11:53:11Z

@markets testing in https://staging.timeoverflow.org I tryed to delete a org as superadmin, but I got a 500. More details here: https://gist.github.com/enricostano/f0b4701a501bc9542ce0ffaa8ada6e63#file-deleting_org_to-L51

markets · 2018-06-07T13:02:26Z

thanks @enricostano! I'll take a look this night, as per your gist, seems something related to new code in develop, I'll rebase and look and it.

Reporting exception: PG::ForeignKeyViolation: ERROR: update or delete on table "members" violates foreign key constraint "events_member_id_fkey" on table "events"
DETAIL: Key (id)=(15) is still referenced from table "events".

enricostano · 2018-06-07T14:18:09Z

@markets yep, it's recent code, but it's already in your branch:
https://github.com/coopdevs/timeoverflow/blob/delete_org/db/schema.rb#L215

Sorry if the issue I detected is not related with your PR, maybe it's already broken in develop 😬

EDIT: I added more details in the issue #359 (comment)

…359, removes all organization memberships and activity when destroying an org instance - introduce OrganizationPolicy to properly define an access control strategy for Organizations

This policy inherits from the ApplicationPolicy class this behaviour: def new? create? end

markets · 2018-06-07T20:53:26Z

Pushed 9418484 (pending final decision in #359) and rebased from develop.

enricostano · 2018-06-08T08:29:48Z

app/models/member.rb

@@ -3,6 +3,7 @@ class Member < ActiveRecord::Base
  belongs_to :organization
  has_one :account, as: :accountable
  has_many :movements, through: :account
+  has_many :events, dependent: :destroy


Would you mind to add this kind of change to the model tests also? Thanks 😍

e.g. https://github.com/coopdevs/timeoverflow/blob/develop/spec/models/member_spec.rb#L8

👉 7f6a087

- added some missing Relations specs - remove a couple of unused Relations

markets · 2018-06-10T19:07:40Z

@enricostano I pushed one commit with more Relation specs and also the dependent: :destroy in Event->PushNotification. Maybe the point of adding the ON DELETE CASCADE for all dependent: :destroy associations deserves a different PR including a general review about this topic?

enricostano · 2018-06-14T13:50:30Z

Thanks @markets, we'll test this (and your other PRs 😬 ) ASAP, we're a bit overwhelmed right now :(

Sorry for the long wait.

enricostano · 2018-08-08T11:12:48Z

@markets could you please rebase from the most recent develop and resolve conflicts please?

Thanks! And sorry again for the long wait...

markets · 2018-08-08T18:01:07Z

No prob @enricostano 👍 rebased from latest develop (and updated Pundit again, they released the v2 some days ago)!

Lot of merges today 👏 👌

…d in controllers/views are never called)

enricostano · 2018-09-17T10:47:42Z

Deployed on https://staging.timeoverflow.org

cc/ @sseerrggii

sseerrggii · 2018-09-18T09:16:11Z

Works fine for me.
Only something weird happens if you delete a organization when you are logged in into this organization. I think current organization it's stored on cookies. In this case we need to log out after organization has been deleted.

markets · 2018-09-19T12:56:48Z

good catch @sseerrggii, I'll send a patch later!

…ation

markets · 2018-09-19T19:49:12Z

@sseerrggii @enricostano fixed here 👉 14fe6bc

sseerrggii · 2018-09-20T08:11:22Z

Hey @markets thanks for looking into it, after delete Organization that is my current organization is doing something diferent but the session still doing strange things... look, this is just after deletion.

I need to remove the cookie to fix it

sauloperez · 2018-09-20T08:23:58Z

So as deleting users and deleting orgs mostly works, I merge this and then we open a new PR to fix that case.

markets · 2018-09-20T22:38:47Z

@sauloperez @sseerrggii definitive fix (I hope) #432

markets added the wip label May 26, 2018

markets mentioned this pull request May 26, 2018

Delete a organization #359

Closed

markets force-pushed the delete_org branch from 928f95d to 66e4faf Compare May 26, 2018 18:25

sauloperez requested changes May 28, 2018

View reviewed changes

markets force-pushed the delete_org branch from 66e4faf to 319bc50 Compare May 28, 2018 18:55

markets removed the wip label May 28, 2018

markets force-pushed the delete_org branch from 2f65ae0 to 2a93b36 Compare May 28, 2018 21:37

markets changed the title ~~Delete a organization (closes #359)~~ Delete a organization + organization policy May 28, 2018

sauloperez approved these changes May 29, 2018

View reviewed changes

markets added wip and removed wip labels May 29, 2018

sauloperez approved these changes May 30, 2018

View reviewed changes

enricostano approved these changes May 30, 2018

View reviewed changes

enricostano added the needs testing label May 30, 2018

enricostano assigned markets Jun 7, 2018

markets added 7 commits June 7, 2018 21:56

- add some missing "dependent: :destroy" in Organization model: fixes #…

1be5c19

…359, removes all organization memberships and activity when destroying an org instance - introduce OrganizationPolicy to properly define an access control strategy for Organizations

gems: update Pundit to latest release (1.1.0)

6c89900

add more OrganizationsController specs

d34e083

UserPolicy minor DRY refactor:

26d77a4

This policy inherits from the ApplicationPolicy class this behaviour: def new? create? end

add some organizations#show view specs

035117b

OrganizatioPolicy: make show action a public page

4bfc636

delete events when deleting target object

9418484

markets force-pushed the delete_org branch from 4fab25c to 9418484 Compare June 7, 2018 20:52

enricostano suggested changes Jun 8, 2018

View reviewed changes

- let's remove PushNotification when we delete an Event

7f6a087

- added some missing Relations specs - remove a couple of unused Relations

markets force-pushed the delete_org branch from 2c61aef to 7f6a087 Compare June 10, 2018 19:18

enricostano approved these changes Jun 14, 2018

View reviewed changes

markets added 2 commits August 8, 2018 19:42

Merge branch 'develop' into delete_org and fix conflicts

0c6c1e7

gems: Pundit 2

2231b63

policies: ✂️ Scopes as we are not really using it (policy_scope metho…

43b3738

…d in controllers/views are never called)

markets mentioned this pull request Sep 6, 2018

Delete user don't work #424

Closed

Merge branch 'develop' into delete_org

7cc1d11

sseerrggii removed the needs testing label Sep 18, 2018

Admin section: Logout user if deleted organization is current_organiz…

14fe6bc

…ation

sseerrggii added the needs testing label Sep 20, 2018

sauloperez merged commit a47597e into develop Sep 20, 2018

sauloperez deleted the delete_org branch September 20, 2018 08:24

sauloperez mentioned this pull request Sep 20, 2018

Release v1.9.0 #429

Merged

markets mentioned this pull request Sep 20, 2018

Definitive fix for deleting org that is current_org #432

Merged

enricostano removed the needs testing label Sep 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete a organization + organization policy #368

Delete a organization + organization policy #368

markets commented May 26, 2018 •

edited

sauloperez May 28, 2018

markets May 28, 2018

sauloperez May 28, 2018

markets May 28, 2018 •

edited

sauloperez May 28, 2018

markets May 28, 2018 •

edited

markets commented May 28, 2018 •

edited

sauloperez May 29, 2018

markets May 29, 2018 •

edited

sseerrggii commented May 29, 2018

markets commented May 29, 2018

markets commented May 29, 2018 •

edited

enricostano commented Jun 7, 2018

markets commented Jun 7, 2018 •

edited

enricostano commented Jun 7, 2018 •

edited

markets commented Jun 7, 2018 •

edited

enricostano Jun 8, 2018 •

edited

markets Jun 10, 2018

markets commented Jun 10, 2018 •

edited

enricostano commented Jun 14, 2018

enricostano commented Aug 8, 2018

markets commented Aug 8, 2018 •

edited

enricostano commented Sep 17, 2018

sseerrggii commented Sep 18, 2018

markets commented Sep 19, 2018

markets commented Sep 19, 2018

sseerrggii commented Sep 20, 2018 •

edited

sauloperez commented Sep 20, 2018

markets commented Sep 20, 2018 •

edited

	def current_member
	@current_member \|\|= current_user.as_member_of(current_organization) if current_user
	end

	def pundit_user
	current_member
	end

Delete a organization + organization policy #368

Delete a organization + organization policy #368

Conversation

markets commented May 26, 2018 • edited

sauloperez May 28, 2018

Choose a reason for hiding this comment

markets May 28, 2018

Choose a reason for hiding this comment

sauloperez May 28, 2018

Choose a reason for hiding this comment

markets May 28, 2018 • edited

Choose a reason for hiding this comment

sauloperez May 28, 2018

Choose a reason for hiding this comment

markets May 28, 2018 • edited

Choose a reason for hiding this comment

markets commented May 28, 2018 • edited

sauloperez May 29, 2018

Choose a reason for hiding this comment

markets May 29, 2018 • edited

Choose a reason for hiding this comment

sseerrggii commented May 29, 2018

markets commented May 29, 2018

markets commented May 29, 2018 • edited

enricostano commented Jun 7, 2018

markets commented Jun 7, 2018 • edited

enricostano commented Jun 7, 2018 • edited

markets commented Jun 7, 2018 • edited

enricostano Jun 8, 2018 • edited

Choose a reason for hiding this comment

markets Jun 10, 2018

Choose a reason for hiding this comment

markets commented Jun 10, 2018 • edited

enricostano commented Jun 14, 2018

enricostano commented Aug 8, 2018

markets commented Aug 8, 2018 • edited

enricostano commented Sep 17, 2018

sseerrggii commented Sep 18, 2018

markets commented Sep 19, 2018

markets commented Sep 19, 2018

sseerrggii commented Sep 20, 2018 • edited

sauloperez commented Sep 20, 2018

markets commented Sep 20, 2018 • edited

markets commented May 26, 2018 •

edited

markets May 28, 2018 •

edited

markets May 28, 2018 •

edited

markets commented May 28, 2018 •

edited

markets May 29, 2018 •

edited

markets commented May 29, 2018 •

edited

markets commented Jun 7, 2018 •

edited

enricostano commented Jun 7, 2018 •

edited

markets commented Jun 7, 2018 •

edited

enricostano Jun 8, 2018 •

edited

markets commented Jun 10, 2018 •

edited

markets commented Aug 8, 2018 •

edited

sseerrggii commented Sep 20, 2018 •

edited

markets commented Sep 20, 2018 •

edited