-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conversation
…anization-id-vs-name - And fix backend compilation due to column renaming
- give explicit name to constraints to make the name more consistent and less implicit
…anization-id-vs-name
…anization-id-vs-name
…anization-id-vs-name
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, should be resolved
There was a problem hiding this 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?
There was a problem hiding this 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
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:
TODOs:
[A-Za-z0-9\-_. ]+
in sql)OrganizationCreationParameters
which has theorganizationDisplayName
prop? -> renaming toorganizationName
? -> its fine, just do it_id_old
entry for the reversion to work.displayName
was renamed toname
). Should a versioned API route be created for this? No need, that route is only used by our frontend.Issues:
fixes Make the organization name its id, drop the mongo id #7689
follow-up: Clean up old organization id residuals #8039
follow-up: Rename organization_name to organization_id in worker args #8038
follow-up (libs): Use webknossos API v8 (organization_id) webknossos-libs#1178