Skip to content

Commit

Permalink
Merge branch 'master' into feat/lemon-multi-select
Browse files Browse the repository at this point in the history
  • Loading branch information
benjackwhite committed Mar 18, 2024
2 parents 790a379 + 144b1fe commit 64c928b
Show file tree
Hide file tree
Showing 84 changed files with 1,296 additions and 1,196 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/build-hogql-parser.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ jobs:

- name: Check if hogql_parser/ has changed
id: changed-files
uses: tj-actions/changed-files@v42
uses: tj-actions/changed-files@v43
with:
since_last_remote_commit: true
files_yaml: |
Expand Down Expand Up @@ -76,7 +76,7 @@ jobs:
python-version: '3.11'

- if: ${{ endsWith(matrix.os, '-arm') }}
uses: deadsnakes/action@v3.0.1 # Unfortunately actions/setup-python@v4 just doesn't work on ARM! This does
uses: deadsnakes/action@v3.1.0 # Unfortunately actions/setup-python@v4 just doesn't work on ARM! This does
with:
python-version: '3.11'

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/container-images-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ jobs:

- name: Check if any Dockerfile has changed
id: changed-files
uses: tj-actions/changed-files@v42
uses: tj-actions/changed-files@v43
with:
files: |
**/Dockerfile
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/stale.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
if: ${{ github.repository == 'PostHog/posthog' }}
runs-on: ubuntu-latest
steps:
- uses: actions/stale@v6
- uses: actions/stale@v9
with:
days-before-issue-stale: 730
days-before-issue-close: 14
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/storybook-chromatic.yml
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ jobs:
VARIANT: ${{ github.event.pull_request.head.repo.full_name == github.repository && 'update' || 'verify' }}
STORYBOOK_SKIP_TAGS: 'test-skip,test-skip-${{ matrix.browser }}'
run: |
pnpm test:visual-regression:ci:$VARIANT --browsers ${{ matrix.browser }} --shard ${{ matrix.shard }}/$SHARD_COUNT
pnpm test:visual:ci:$VARIANT --browsers ${{ matrix.browser }} --shard ${{ matrix.shard }}/$SHARD_COUNT
- name: Archive failure screenshots
if: ${{ failure() }}
Expand Down
75 changes: 41 additions & 34 deletions .storybook/test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,27 @@ import { StoryContext } from '@storybook/csf'

// 'firefox' is technically supported too, but as of June 2023 it has memory usage issues that make is unusable
type SupportedBrowserName = 'chromium' | 'webkit'
type SnapshotTheme = 'legacy' | 'light' | 'dark'
type SnapshotTheme = 'light' | 'dark'

// Extend Storybook interface `Parameters` with Chromatic parameters
declare module '@storybook/types' {
interface Parameters {
options?: any
/** @default 'padded' */
layout?: 'padded' | 'fullscreen' | 'centered'
testOptions?: {
/**
* Whether we should wait for all loading indicators to disappear before taking a snapshot.
* @default true
*/
waitForLoadersToDisappear?: boolean
/** If set, we'll wait for the given selector to be satisfied. */
waitForSelector?: string
/** If set, we'll wait for the given selector (or all selectors, if multiple) to be satisfied. */
waitForSelector?: string | string[]
/**
* Whether navigation (sidebar + topbar) should be excluded from the snapshot.
* Warning: Fails if enabled for stories in which navigation is not present.
* Whether navigation should be included in the snapshot. Only applies to `layout: 'fullscreen'` stories.
* @default false
*/
excludeNavigationFromSnapshot?: boolean
includeNavigationInSnapshot?: boolean
/**
* The test will always run for all the browers, but snapshots are only taken in Chromium by default.
* Override this to take snapshots in other browsers too.
Expand All @@ -48,13 +48,13 @@ declare module '@storybook/types' {
}
}

const RETRY_TIMES = 3
const RETRY_TIMES = 2
const LOADER_SELECTORS = [
'.ant-skeleton',
'.Spinner',
'.LemonSkeleton',
'.LemonTableLoader',
'.Toastify__toast-container',
'.Toastify__toast',
'[aria-busy="true"]',
'.SessionRecordingPlayer--buffering',
'.Lettermark--unknown',
Expand All @@ -65,13 +65,26 @@ const customSnapshotsDir = `${process.cwd()}/frontend/__snapshots__`
const JEST_TIMEOUT_MS = 15000
const PLAYWRIGHT_TIMEOUT_MS = 10000 // Must be shorter than JEST_TIMEOUT_MS

const ATTEMPT_COUNT_PER_ID: Record<string, number> = {}

module.exports = {
setup() {
expect.extend({ toMatchImageSnapshot })
jest.retryTimes(RETRY_TIMES, { logErrorsBeforeRetry: true })
jest.setTimeout(JEST_TIMEOUT_MS)
},
async postVisit(page, context) {
ATTEMPT_COUNT_PER_ID[context.id] = (ATTEMPT_COUNT_PER_ID[context.id] || 0) + 1
await page.evaluate(
([retry, id]) => console.log(`[${id}] Attempt ${retry}`),
[ATTEMPT_COUNT_PER_ID[context.id], context.id]
)
if (ATTEMPT_COUNT_PER_ID[context.id] > 1) {
// When retrying, resize the viewport and then resize again to default,
// just in case the retry is due to a useResizeObserver fail
await page.setViewportSize({ width: 1920, height: 1080 })
await page.setViewportSize({ width: 1280, height: 720 })
}
const browserContext = page.context()
const storyContext = await getStoryContext(page, context)
const { snapshotBrowsers = ['chromium'] } = storyContext.parameters?.testOptions ?? {}
Expand All @@ -96,7 +109,7 @@ async function expectStoryToMatchSnapshot(
const {
waitForLoadersToDisappear = true,
waitForSelector,
excludeNavigationFromSnapshot = false,
includeNavigationInSnapshot = false,
} = storyContext.parameters?.testOptions ?? {}

let check: (
Expand All @@ -107,26 +120,29 @@ async function expectStoryToMatchSnapshot(
targetSelector?: string
) => Promise<void>
if (storyContext.parameters?.layout === 'fullscreen') {
if (excludeNavigationFromSnapshot) {
check = expectStoryToMatchSceneSnapshot
if (includeNavigationInSnapshot) {
check = expectStoryToMatchViewportSnapshot
} else {
check = expectStoryToMatchFullPageSnapshot
check = expectStoryToMatchSceneSnapshot
}
} else {
check = expectStoryToMatchComponentSnapshot
}

await waitForPageReady(page)
await page.evaluate(() => {
// Stop all animations for consistent snapshots
await page.evaluate((layout: string) => {
// Stop all animations for consistent snapshots, and adjust other styles
document.body.classList.add('storybook-test-runner')
})
document.body.classList.add(`storybook-test-runner--${layout}`)
}, storyContext.parameters?.layout || 'padded')
if (waitForLoadersToDisappear) {
// The timeout is reduced so that we never allow toasts – they usually signify something wrong
await page.waitForSelector(LOADER_SELECTORS.join(','), { state: 'detached', timeout: 1000 })
await page.waitForSelector(LOADER_SELECTORS.join(','), { state: 'detached', timeout: 3000 })
}
if (waitForSelector) {
if (typeof waitForSelector === 'string') {
await page.waitForSelector(waitForSelector)
} else if (Array.isArray(waitForSelector)) {
await Promise.all(waitForSelector.map((selector) => page.waitForSelector(selector)))
}

await page.waitForTimeout(400) // Wait for effects to finish
Expand All @@ -151,7 +167,7 @@ async function expectStoryToMatchSnapshot(
await check(page, context, browser, 'dark', storyContext.parameters?.testOptions?.snapshotTargetSelector)
}

async function expectStoryToMatchFullPageSnapshot(
async function expectStoryToMatchViewportSnapshot(
page: Page,
context: TestContext,
browser: SupportedBrowserName,
Expand All @@ -166,12 +182,10 @@ async function expectStoryToMatchSceneSnapshot(
browser: SupportedBrowserName,
theme: SnapshotTheme
): Promise<void> {
await page.evaluate(() => {
// The screenshot gets clipped by overflow hidden on .Navigation3000
document.querySelector('Navigation3000')?.setAttribute('style', 'overflow: visible;')
})

await expectLocatorToMatchStorySnapshot(page.locator('main'), context, browser, theme)
// If the `main` element isn't present, let's use `body` - this is needed in logged-out screens.
// We use .last(), because the order of selector matches is based on the order of elements in the DOM,
// and not the order of the selectors in the query.
await expectLocatorToMatchStorySnapshot(page.locator('body, main').last(), context, browser, theme)
}

async function expectStoryToMatchComponentSnapshot(
Expand All @@ -181,13 +195,11 @@ async function expectStoryToMatchComponentSnapshot(
theme: SnapshotTheme,
targetSelector: string = '#storybook-root'
): Promise<void> {
await page.evaluate((theme) => {
await page.evaluate(() => {
const rootEl = document.getElementById('storybook-root')
if (!rootEl) {
throw new Error('Could not find root element')
}
// Make the root element (which is the default screenshot reference) hug the component
rootEl.style.display = 'inline-block'
// If needed, expand the root element so that all popovers are visible in the screenshot
document.querySelectorAll('.Popover').forEach((popover) => {
const currentRootBoundingClientRect = rootEl.getBoundingClientRect()
Expand All @@ -205,9 +217,7 @@ async function expectStoryToMatchComponentSnapshot(
rootEl.style.width = `${-popoverBoundingClientRect.left + currentRootBoundingClientRect.right}px`
}
})
// For legacy style, make the body transparent to take the screenshot without background
document.body.style.background = theme === 'legacy' ? 'transparent' : 'var(--bg-3000)'
}, theme)
})

await expectLocatorToMatchStorySnapshot(page.locator(targetSelector), context, browser, theme, {
omitBackground: true,
Expand All @@ -222,10 +232,7 @@ async function expectLocatorToMatchStorySnapshot(
options?: LocatorScreenshotOptions
): Promise<void> {
const image = await locator.screenshot({ ...options })
let customSnapshotIdentifier = context.id
if (theme !== 'legacy') {
customSnapshotIdentifier += `--${theme}`
}
let customSnapshotIdentifier = `${context.id}--${theme}`
if (browser !== 'chromium') {
customSnapshotIdentifier += `--${browser}`
}
Expand Down
2 changes: 1 addition & 1 deletion ee/api/dashboard_collaborator.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class DashboardCollaboratorViewSet(
mixins.DestroyModelMixin,
viewsets.GenericViewSet,
):
scope_object = "INTERNAL"
scope_object = "dashboard"
permission_classes = [CanEditDashboardCollaborator]
pagination_class = None
queryset = DashboardPrivilege.objects.select_related("dashboard").filter(user__is_active=True)
Expand Down
15 changes: 10 additions & 5 deletions ee/api/role.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from ee.models.feature_flag_role_access import FeatureFlagRoleAccess
from ee.models.organization_resource_access import OrganizationResourceAccess
from ee.models.role import Role, RoleMembership
from posthog.api.organization_member import OrganizationMemberSerializer
from posthog.api.routing import TeamAndOrgViewSetMixin
from posthog.api.shared import UserBasicSerializer
from posthog.models import OrganizationMembership
Expand Down Expand Up @@ -105,20 +106,24 @@ def get_queryset(self):

class RoleMembershipSerializer(serializers.ModelSerializer):
user = UserBasicSerializer(read_only=True)
organization_member = OrganizationMemberSerializer(read_only=True)
role_id = serializers.UUIDField(read_only=True)
user_uuid = serializers.UUIDField(required=True, write_only=True)

class Meta:
model = RoleMembership
fields = ["id", "role_id", "user", "joined_at", "updated_at", "user_uuid"]

read_only_fields = ["id", "role_id", "user"]
fields = ["id", "role_id", "organization_member", "user", "joined_at", "updated_at", "user_uuid"]
read_only_fields = ["id", "role_id", "organization_member", "user", "joined_at", "updated_at"]

def create(self, validated_data):
user_uuid = validated_data.pop("user_uuid")
try:
validated_data["user"] = User.objects.filter(is_active=True).get(uuid=user_uuid)
except User.DoesNotExist:
validated_data["organization_member"] = OrganizationMembership.objects.select_related("user").get(
organization_id=self.context["organization_id"], user__uuid=user_uuid, user__is_active=True
)

validated_data["user"] = validated_data["organization_member"].user
except OrganizationMembership.DoesNotExist:
raise serializers.ValidationError("User does not exist.")
validated_data["role_id"] = self.context["role_id"]
try:
Expand Down
57 changes: 46 additions & 11 deletions ee/api/test/test_role_membership.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,50 @@ def setUp(self):
self.eng_role = Role.objects.create(name="Engineering", organization=self.organization)
self.marketing_role = Role.objects.create(name="Marketing", organization=self.organization)

def test_adds_member_to_a_role(self):
user = User.objects.create_and_join(self.organization, "[email protected]", None)

self.organization_membership.level = OrganizationMembership.Level.ADMIN
self.organization_membership.save()
assert RoleMembership.objects.count() == 0

res = self.client.post(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships",
{"user_uuid": user.uuid},
)

assert res.status_code == status.HTTP_201_CREATED
assert res.json()["id"] == str(RoleMembership.objects.first().id)
assert res.json()["role_id"] == str(self.eng_role.id)
assert res.json()["organization_member"]["user"]["id"] == user.id
assert res.json()["user"]["id"] == user.id

def test_only_organization_admins_and_higher_can_add_users(self):
user_a = User.objects.create_and_join(self.organization, "[email protected]", None)
user_b = User.objects.create_and_join(self.organization, "[email protected]", None)
self.assertEqual(self.organization_membership.level, OrganizationMembership.Level.MEMBER)
assert self.organization_membership.level == OrganizationMembership.Level.MEMBER

add_user_b_res = self.client.post(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships",
{"user_uuid": user_b.uuid},
)
self.assertEqual(add_user_b_res.status_code, status.HTTP_403_FORBIDDEN)
assert add_user_b_res.status_code == status.HTTP_403_FORBIDDEN

self.organization_membership.level = OrganizationMembership.Level.ADMIN
self.organization_membership.save()
add_user_a_res = self.client.post(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships",
{"user_uuid": user_a.uuid},
)
self.assertEqual(add_user_a_res.status_code, status.HTTP_201_CREATED)
self.assertEqual(RoleMembership.objects.count(), 1)
self.assertEqual(RoleMembership.objects.first().user, user_a) # type: ignore
assert add_user_a_res.status_code == status.HTTP_201_CREATED
assert RoleMembership.objects.count() == 1
assert RoleMembership.objects.first().user == user_a

def test_user_can_belong_to_multiple_roles(self):
user_a = User.objects.create_and_join(self.organization, "[email protected]", None)
self.organization_membership.level = OrganizationMembership.Level.ADMIN
self.organization_membership.save()
self.assertEqual(RoleMembership.objects.count(), 0)
assert RoleMembership.objects.count() == 0

self.client.post(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships",
Expand All @@ -47,7 +65,24 @@ def test_user_can_belong_to_multiple_roles(self):
f"/api/organizations/@current/roles/{self.marketing_role.id}/role_memberships",
{"user_uuid": user_a.uuid},
)
self.assertEqual(RoleMembership.objects.count(), 2)
assert RoleMembership.objects.count() == 2

def test_user_can_be_removed_from_role(self):
user_a = User.objects.create_and_join(self.organization, "[email protected]", None)
self.organization_membership.level = OrganizationMembership.Level.ADMIN
self.organization_membership.save()
assert RoleMembership.objects.count() == 0

res = self.client.post(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships",
{"user_uuid": user_a.uuid},
)
assert RoleMembership.objects.count() == 1
delete_response = self.client.delete(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships/{res.json()['id']}",
)
assert delete_response.status_code == status.HTTP_204_NO_CONTENT
assert RoleMembership.objects.count() == 0

def test_returns_correct_results_by_organization(self):
self.organization_membership.level = OrganizationMembership.Level.ADMIN
Expand All @@ -62,10 +97,10 @@ def test_returns_correct_results_by_organization(self):
)
other_org_same_name_role = Role.objects.create(organization=other_org, name="Engineering")
RoleMembership.objects.create(role=other_org_same_name_role, user=user_b)
self.assertEqual(RoleMembership.objects.count(), 2)
assert RoleMembership.objects.count() == 2
get_res = self.client.get(
f"/api/organizations/@current/roles/{self.eng_role.id}/role_memberships",
)
self.assertEqual(get_res.json()["count"], 1)
self.assertEqual(get_res.json()["results"][0]["user"]["distinct_id"], user_a.distinct_id)
self.assertNotContains(get_res, str(user_b.email))
assert get_res.json()["count"] == 1
assert get_res.json()["results"][0]["user"]["distinct_id"] == user_a.distinct_id
assert str(user_b.email) not in get_res.content.decode()
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# serializer version: 1
# name: ClickhouseTestExperimentSecondaryResults.test_basic_secondary_metric_results
'''
/* user_id:108 celery:posthog.tasks.tasks.sync_insight_caching_state */
/* user_id:107 celery:posthog.tasks.tasks.sync_insight_caching_state */
SELECT team_id,
date_diff('second', max(timestamp), now()) AS age
FROM events
Expand Down
Loading

0 comments on commit 64c928b

Please sign in to comment.