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

fix: early access management cancel button #17857

Merged
merged 5 commits into from
Oct 12, 2023
Merged

fix: early access management cancel button #17857

merged 5 commits into from
Oct 12, 2023

Conversation

kiran-4444
Copy link
Contributor

Problem

Fixes: #17841

Changes

  • image

  • Also, fixed "cancel" button behaviour while creating and editing early access feature

How did you test this code?

Locally in my browser

@Justicea83
Copy link
Contributor

I think you need to write some form of tests for this changes

@kiran-4444
Copy link
Contributor Author

Can you point me to an example of these type of tests? I'll write those tests based on that.

@neilkakkar
Copy link
Collaborator

re: tests, seems like we're missing all e2e tests, but if you wanted to write these (would appreciate it), here's one you can follow: https://github.com/PostHog/posthog/blob/master/cypress/e2e/cohorts.cy.ts#L1 - to create cypress tests for early access feature.

@kiran-4444
Copy link
Contributor Author

Sure! this is my first time looking at tests on frontend side. I'll go through the reference you sent and will start. Should be fun adding these tests to early access feature!

@neilkakkar
Copy link
Collaborator

perfect :) -> https://posthog.com/handbook/engineering/developing-locally#end-to-end

@kiran-4444
Copy link
Contributor Author

kiran-4444 commented Oct 9, 2023

I'm trying to run cohorts e2e tests, are they passing (I can see the green tick though)? Or are they failing? (not sure but telling this by seeing those red boxes)

image

Also I started writing a sample e2e test for early access features (by looking cohorts e2e tests) like this (I want these to fail so I changes management to managements):

describe("Early Access Management", () => {
    beforeEach(() => {
        cy.visit("/early_access_features")
    })

    it('Early Access Management', () => {
        cy.get('h1').should('contain', 'Early Access Managements')
        cy.title().should('equal', 'Early Access Managements • PostHog')
    })

})

But it seems like I'm making a mistake cause I'm always getting a green tick in cypress app no matter what. Not sure what's the issue.

EDIT: My bad, I didn't run migrations properly and hence cannot login.

@neilkakkar
Copy link
Collaborator

My bad, I didn't run migrations properly and hence cannot login.

Did you manage to get these working now?

@kiran-4444
Copy link
Contributor Author

Did you manage to get these working now?

Yes.

@Justicea83
Copy link
Contributor

Looks great!!

@kiran-4444
Copy link
Contributor Author

WARNINGS:
?: (axes.W001) You are using the django-axes cache handler for login attempt tracking. Your cache configuration is however invalid and will not work correctly with django-axes. This can leave security holes in your login systems as attempts are not tracked correctly. Reconfigure settings.AXES_CACHE and settings.CACHES per django-axes configuration documentation.
Traceback (most recent call last):
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 362, in _get_applied_migrations_and_create_tables
    return self._get_applied_migrations(migrations_package_name, replicated)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 379, in _get_applied_migrations
    return set(obj.module_name for obj in self.select(query))
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 379, in <genexpr>
    return set(obj.module_name for obj in self.select(query))
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 276, in select
    r = self._send(query, settings, True)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 389, in _send
    raise ServerError(r.text)
infi.clickhouse_orm.database.ServerError: Code: 60. DB::Exception: Table posthog_test.infi_clickhouse_orm_migrations_distributed doesn't exist. (UNKNOWN_TABLE) (version 23.6.1.1524 (official build))
 (0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/luna/Documents/posthog/manage.py", line 21, in <module>
    main()
  File "/Users/luna/Documents/posthog/manage.py", line 17, in main
    execute_from_command_line(sys.argv)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/django/core/management/__init__.py", line 413, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/django/core/management/base.py", line 354, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/django/core/management/base.py", line 398, in execute
    output = self.handle(*args, **options)
  File "/Users/luna/Documents/posthog/posthog/management/commands/migrate_clickhouse.py", line 36, in handle
    self.migrate(CLICKHOUSE_HTTP_URL, options)
  File "/Users/luna/Documents/posthog/posthog/management/commands/migrate_clickhouse.py", line 74, in migrate
    database.migrate(MIGRATIONS_PACKAGE_NAME, options["upto"], replicated=True)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 346, in migrate
    applied_migrations = self._get_applied_migrations_and_create_tables(migrations_package_name, replicated=replicated)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 366, in _get_applied_migrations_and_create_tables
    self.create_table(MigrationHistoryReplicated)
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 152, in create_table
    self._send(model_class.create_table_sql(self))
  File "/Users/luna/Documents/posthog/venv/lib/python3.10/site-packages/infi/clickhouse_orm/database.py", line 389, in _send
    raise ServerError(r.text)
infi.clickhouse_orm.database.ServerError: Code: 253. DB::Exception: There was an error on [localhost:9000]: Code: 253. DB::Exception: Replica /clickhouse/prod/tables/noshard/posthog_test/infi_clickhouse_orm_migrations/replicas/ch1-01 already exists. (REPLICA_ALREADY_EXISTS) (version 23.6.1.1524 (official build)). (REPLICA_ALREADY_EXISTS) (version 23.6.1.1524 (official build))
 (0)
[1]    32151 terminated  ./bin/e2e-test-runner

I thought I got that right yesterday. But when I try to execute the same script (e2e-test-runner), I ran into this error. Even if this didn't show up and I try to run tests in cypress app, I get 400 for /api/login. How to correctly run that e2e-test-runner script and not run into these DB related issues?

@neilkakkar
Copy link
Collaborator

I'm not sure how you got into that state, but if you docker compose down, i.e. nuke everything, and start from a fresh install, just running ./bin/e2e-test-runner should setup everything correctly for you.

@kiran-4444
Copy link
Contributor Author

kiran-4444 commented Oct 11, 2023

I'm not sure how you got into that state, but if you docker compose down, i.e. nuke everything, and start from a fresh install, just running ./bin/e2e-test-runner should setup everything correctly for you.

Yeah, tried that. Now it's taking a lot of time for login.

image

I checked if backend is running by hitting localhost:8080 and it's running but I just can't login through cypress.

@kiran-4444
Copy link
Contributor Author

kiran-4444 commented Oct 11, 2023

I'm not sure how you got into that state, but if you docker compose down, i.e. nuke everything, and start from a fresh install, just running ./bin/e2e-test-runner should setup everything correctly for you.

This and,

responseTimeout: 80000,

Adding this to cypress config solved my login issue.

thanks!

@kiran-4444
Copy link
Contributor Author

Hey Neil, I completed e2e tests for this. Let me know if anything's missing.

@neilkakkar
Copy link
Collaborator

very nice, this is looking great, thanks @kiran-4444 - just waiting for tests to pass before I merge :)

Would love to send you a posthog merch code for your contribution - send me an email at neil @ posthog . com (without the spaces)

@neilkakkar neilkakkar self-assigned this Oct 12, 2023
@neilkakkar neilkakkar merged commit 45d5794 into PostHog:master Oct 12, 2023
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Early Access Management issues
3 participants