Add organization settings #3324

Twixes · 2021-02-12T11:37:03Z

Changes

This reworks Organization Members to more robust Organization Settings.

Checklist

All querysets/queries filter by Organization, by Team, and by User
Django backend tests

paolodamico

❤️ , fixed merge conflicts & cypress tests. Some minor comments but otherwise good to go.

This looks confusing when you don't have any invites, I'd add some empty state here.

paolodamico · 2021-03-01T03:02:07Z

ee/api/test/test_organization.py

-        )
-        self.assertEqual(response.status_code, 400)
-        self.assertTrue(Organization.objects.filter(id=org_id).exists())
+        self.assertEqual(response.status_code, 204)


If we can now leave the user, organization-less, maybe worth changing the test name here?

paolodamico · 2021-03-01T03:05:03Z

frontend/src/scenes/organizationLogic.tsx

-                createOrganization: async (name: string) => await api.create('api/organizations/', { name }),
+                createOrganization: async (name: string) => {
+                    const result = await api.create('api/organizations/', { name })
+                    teamLogic.actions.loadCurrentTeam()


You're re-loading these endpoints, but there's still a listener that will cause a hard refresh, rendering this unused.

I'll keep this as we'll be moving towards not requiring a hard refresh (#3421).

paolodamico · 2021-03-01T03:06:23Z

frontend/src/scenes/organizationLogic.tsx

        createOrganizationSuccess: () => {
            window.location.href = '/organization/members'
        },
-    },
+        renameCurrentOrganizationSuccess: () => {
+            toast.success('Organization has been renamed')


Maybe it should be a larger question than the scope of this PR, but not showing an error message when something went wrong is not good UX.

Ah, previously we relied on automatic API error toasts, but these got slashed and now lots of places in the UI are confusing because there's no info when something goes wrong…

Maybe we need to start doing case-by-case error handling? Not entirely sure about the best approach here

This is kind of pending #3525 and our overall approach to errors. I'm still for blanket catch-and-notify, unsure what (and whether) to do here.

paolodamico · 2021-03-01T03:07:28Z

frontend/src/scenes/project/Settings/index.tsx

+            <PageHeader
+                title="Project Settings"
+                caption={`Organize your analytics within the project. These settings only apply to ${
+                    currentTeam?.name ?? '–'


how about a more applicable placeholder, "the current project"?

posthog/api/organization.py

frontend/src/scenes/organization/Settings/Members.tsx

paolodamico · 2021-03-01T03:50:17Z

frontend/src/lib/components/RestrictedArea.tsx

+            if (minimumAccessLevel === OrganizationMembershipLevel.Owner) {
+                return 'This area is restricted to the organization owner.'
+            }
+            return `This area is restricted to organization ${OrganizationMembershipLevel[minimumAccessLevel]}s and up. Your level is only ${currentOrganization.membership_level}.`


Nit: think I would drop the only, maybe sounds a bit aggressive?

Twixes · 2021-03-09T12:13:21Z

How about this empty state?

Up for re-review @paolodamico.

paolodamico

Some more feedback, feel free to disregard if you don't think it's useful.

Tried deleting a project, got this server error.

django.db.utils.IntegrityError: update or delete on table "posthog_team" violates foreign key constraint "posthog_elementgroup_team_id_3ced0286_fk_posthog_team_id" on table "posthog_elementgroup"

In terms of UX when you're deleting a project. After you click on delete, the buttons stay enabled and the modal can be dismissed. The toast says project is being deleted but then I get no confirmation. Further, if something went wrong I get no error message, the app just freezes there.
The new avatar image on invites shows a ? (presumably because first name is not set), we could use the email as a fallback.

paolodamico · 2021-03-10T01:03:02Z

ee/api/test/test_organization.py

+            else:
+                potential_err_message = f"Somehow did not delete the org as a level {level} (which is owner)"
+                self.assertEqual(response.status_code, 204, potential_err_message)
+                self.assertTrue(self.organization.name, "Woof")


If the organization is deleted, it shouldn't exist. I think you need to assert that the object does not exist in the DB (filtering by pk). As an alternative you can assertRaises calling object.refresh_from_db()

paolodamico · 2021-03-10T01:04:00Z

ee/api/test/test_organization.py

+
+        self.assertEqual(response_bis.status_code, 404, "Did not return a 404 on trying to delete a nonexistent org")
+
+    def test_no_delete_organization_not_owning(self):


I would separate this into two tests, or remove the owner case altogether, makes the code confusing, and besides if the order of levels changes and owner is not last, this will lead to weird results.

paolodamico · 2021-03-10T01:04:44Z

ee/api/test/test_organization.py

+                self.assertTrue(self.organization.name, self.CONFIG_ORGANIZATION_NAME)
+            else:
+                potential_err_message = f"Somehow did not delete the org as a level {level} (which is owner)"
+                self.assertEqual(response.status_code, 204, potential_err_message)


Maybe worth asserting that elements that should be cascade-deleted are removed too (e.g. OrgnizationMemberships). Also, assert elements that should not be deleted (e.g. Users)

paolodamico · 2021-03-10T01:09:36Z

frontend/src/scenes/organization/Settings/DangerZone.tsx

+            okType: 'danger',
+            okButtonProps: {
+                // @ts-expect-error - data-attr works just fine despite not being in ButtonProps
+                'data-attr': 'delete-organization-ok',


I think it's worth adding a custom event when an organization is deleted, we could include too number of projects, members, persons, and events?

Alright, used get_analytics_metadata for this. Without events though, as they are more difficult to compute across Postgres and CH.

Twixes · 2021-03-10T12:09:30Z

Addressed all. I'll go ahead and merge. :)

Add organization settings

73860e4

timgl temporarily deployed to posthog-org-settings-y314lbpj1 February 12, 2021 11:40 Inactive

Update test_organization.py

c42350f

Twixes temporarily deployed to posthog-org-settings-y314lbpj1 February 12, 2021 14:10 Inactive

Fix TS issues

d34b6fd

Twixes temporarily deployed to posthog-org-settings-y314lbpj1 February 12, 2021 19:32 Inactive

paolodamico mentioned this pull request Feb 15, 2021

Add team members to main nav #3342

Closed

4 tasks

Improve permissioning frontend

35870d5

Twixes temporarily deployed to posthog-org-settings-y314lbpj1 February 17, 2021 13:55 Inactive

Merge branch 'master' into org-settings

f695da3

Twixes had a problem deploying to posthog-org-settings-y314lbpj1 February 17, 2021 14:15 Failure

Fix an import

bbde3d4

Twixes temporarily deployed to posthog-org-settings-y314lbpj1 February 17, 2021 14:28 Inactive

Twixes marked this pull request as ready for review February 26, 2021 15:16

Twixes requested a review from paolodamico February 26, 2021 15:16

Twixes mentioned this pull request Feb 26, 2021

Release 1.23.0 – 15 March 2021 #3423

Closed

paolodamico added 3 commits February 28, 2021 19:00

Merge branch 'master' into org-settings

d4b4bf3

minor typos

1bab8cd

fix cypress

b809ef8

paolodamico suggested changes Mar 1, 2021

View reviewed changes

Twixes added 9 commits March 9, 2021 02:27

Make use of AnalyticsDestroyModelMixin

1762031

Solve nits

cbce464

Merge branch 'master' into org-settings

032c2a5

Rework Invites empty state

91150e5

Update organization test

3164a77

Improve organization tests and permissioning

e58f2d6

Polish Organization Settings frontend

70a2f8e

Improve Members UX

9e9f2da

Use RestrictedArea in Project Settings

32a2f8f

Twixes requested a review from paolodamico March 9, 2021 11:45

Fix APITestMixin._create_user

305361d

Twixes temporarily deployed to posthog-org-settings-jqqv4q6a6 March 9, 2021 14:11 Inactive

some minor copy adjustments

46a4683

paolodamico reviewed Mar 10, 2021

View reviewed changes

timgl temporarily deployed to posthog-org-settings-jqqv4q6a6 March 10, 2021 01:24 Inactive

Twixes added 4 commits March 10, 2021 11:05

Use email initial in ProfilePicture

8d1846c

Clean up org deletion tests

66da598

Improve org/project deletion UX and reliability

c426205

Enhance org deletion test

613995a

Twixes temporarily deployed to posthog-org-settings-jqqv4q6a6 March 10, 2021 11:54 Inactive

Twixes merged commit e50d591 into master Mar 10, 2021

Twixes deleted the org-settings branch March 10, 2021 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add organization settings #3324

Add organization settings #3324

Twixes commented Feb 12, 2021 •

edited

paolodamico left a comment

paolodamico Mar 1, 2021

paolodamico Mar 1, 2021

Twixes Mar 9, 2021

paolodamico Mar 1, 2021

Twixes Mar 1, 2021

paolodamico Mar 1, 2021

Twixes Mar 9, 2021

paolodamico Mar 1, 2021

paolodamico Mar 1, 2021

Twixes commented Mar 9, 2021

paolodamico left a comment

paolodamico Mar 10, 2021

paolodamico Mar 10, 2021

paolodamico Mar 10, 2021

paolodamico Mar 10, 2021

Twixes Mar 10, 2021 •

edited

Twixes commented Mar 10, 2021


		self.assertEqual(response_bis.status_code, 404, "Did not return a 404 on trying to delete a nonexistent org")

		def test_no_delete_organization_not_owning(self):

Add organization settings #3324

Add organization settings #3324

Conversation

Twixes commented Feb 12, 2021 • edited

Changes

Checklist

paolodamico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes commented Mar 9, 2021

paolodamico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes Mar 10, 2021 • edited

Choose a reason for hiding this comment

Twixes commented Mar 10, 2021

Twixes commented Feb 12, 2021 •

edited

Twixes Mar 10, 2021 •

edited