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

Make organization name its id, drop the mongo id #7386

Merged
merged 54 commits into from
Aug 29, 2024

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Oct 15, 2023

The old organizationId (which was a mongoId/ObjectId) is dropped. The old organizationName is renamed to organizationId. The old organizationDisplayName is renamed to organizationName.

URL of deployed dev instance (used for testing):

Steps to test:

  • Test annotating, annotation download+reupload, signup, login
  • Test task system
  • Test libs access
  • Test migrations

TODOs:

  • update in sql schema
  • update in scala code
  • the mongoid is used as key in loki?
  • update in routes
  • update in proto
  • update in frontend
  • checks on name in sql and create organization code (allow [A-Za-z0-9\-_. ]+ in sql)
  • test around
  • sql migration, careful with foreign keys
    • to make rollback easier, keep old ids, rename to nullable unused column?
    • migrate analytics events json in postgres
      • for foreign-webknossos analytics events it may be that the id cannot be looked up. talk to norman on how to handle these.
  • make sure that analytics events those from old webknossoses can still be ingested
  • make sure that analytics events those from new webknossoses can still be ingested
  • bump api version, provide adapters → only for routes used by python client. e.g. dataset_list
  • todos in code
  • change worker job arguments from organization_name to organization_id: Follow-up Rename organization_name to organization_id in worker args #8038
  • adapt vx runs moving script in notion, make it more robust -> can be done later
  • What do to with OrganizationCreationParameters which has the organizationDisplayName prop? -> renaming to organizationName? -> its fine, just do it
  • check whether frontend events eventProperties need to be migrated
  • Mention in reversion that the newly created orgas first need a manual valid _id_old entry for the reversion to work.
  • The update route for organizations was changed. (The param displayName was renamed to name). Should a versioned API route be created for this? No need, that route is only used by our frontend.
  • Test wk thoroughly

Issues:


@fm3 fm3 self-assigned this Oct 15, 2023
@fm3 fm3 marked this pull request as ready for review August 29, 2024 07:35
@fm3 fm3 changed the title WIP: make organization name its id, drop the mongo id Make organization name its id, drop the mongo id Aug 29, 2024
@@ -685,9 +686,9 @@ CREATE TABLE webknossos.analyticsEvents(
created TIMESTAMPTZ NOT NULL DEFAULT NOW(),
sessionId BIGINT NOT NULL,
eventType VARCHAR(512) NOT NULL,
eventProperties JSONB NOT NULL,
eventProperties JSONB NOT NULL, -- TODO migrate user json from old to new organization id
Copy link
Member Author

Choose a reason for hiding this comment

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

@MichaelBuessemeyer is this TODO still open?

Copy link
Contributor

Choose a reason for hiding this comment

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

nope, should be resolved

Copy link
Member Author

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

LGTM :)

Two questions:

  • There is an open TODO comment in schema.sql, I think it is actually resolved. Can you double-check that?
  • I think I remember you talking with Norman about the idea of a full backup of the analyticsEvents table. That seems to not be included in the evolution. Does that plan still stand? If so, do we want to do this manually or do we want to add it to the evolution?

I fixed a couple of small things in my last commits.

I can’t hit approve since I was the original author. Do you want to do so?

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

LGTM checked newest changes together with @fm3

@MichaelBuessemeyer MichaelBuessemeyer merged commit 6c6ec24 into master Aug 29, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the organization-id-vs-name branch August 29, 2024 12:07
@fm3 fm3 mentioned this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the organization name its id, drop the mongo id
3 participants