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

Add owner information to organization #6811

Merged
merged 26 commits into from
Feb 8, 2023
Merged

Add owner information to organization #6811

merged 26 commits into from
Feb 8, 2023

Conversation

hotzenklotz
Copy link
Member

@hotzenklotz hotzenklotz commented Feb 3, 2023

  • Added owner information to organization REST API route and organization page.
    • Flo and I decided that we did not want share the whole orga owner user object through the API but rather just a string since some API calls expose the default orga without any authentication
  • Also added the owner name to some of the pricing tooltips to help guide people to right person.
  • Added a button to the tooltip/popover in case you are the owner to upgrade seamlessly.

Steps to test:

  • Hover over any feature that is restricted to certain plans
  • Check out the owner name in the organization page

Issues:


(Please delete unneeded items, merge only when none are left open)

@hotzenklotz hotzenklotz requested review from fm3 and philippotto and removed request for fm3 February 6, 2023 16:35
@hotzenklotz
Copy link
Member Author

@fm3 Please review backend parts
@philippotto Please review frontend parts.

…a_owner

* 'master' of github.com:scalableminds/webknossos:
  Rewrite database tooling and support for postgres passwords (#6803)
@hotzenklotz hotzenklotz requested a review from fm3 February 6, 2023 16:38
@@ -299,6 +299,7 @@ function UpgradePricingPlanModal({
) : null}
</>
}
zIndex={10000} // overlay everything
Copy link
Member Author

Choose a reason for hiding this comment

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

@philippotto I tried to click somewhere in the background (document.body.click()) to make the menu disappear but that did not work. With the high zIndex you can at least be sure that it overlays anything.

@hotzenklotz hotzenklotz changed the title Add owner information to organization (WIP) Add owner information to organization Feb 6, 2023
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

👍 Only left some nitpicks/suggestions. Will test later.

requiredPlan: string,
organizationOwnerName: string,
) =>
`This feature is not available in your organization's plan. Ask your organization owner ${organizationOwnerName} to upgrade to at least a ${requiredPlan} plan.`,
Copy link
Member

Choose a reason for hiding this comment

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

"Ask the owner of your organization (John Doe)" would be more idiomatic english than "organization owner", I think.
Also, I'd change the type to organizationOwnerName: string | null and then do a conditional on that so that the double space is avoided if the owner is not known.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I'd change the type to organizationOwnerName: string | null and then do a conditional on that so that the double space is avoided if the owner is not known.

Mhm, I was hoping that getFeatureNotAvailabeInPlanMessage() already checks against null to avoid doing this logic in the messages module. it, does not, however, prevent a double space.

Copy link
Member

Choose a reason for hiding this comment

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

ok, i see. i don't even know whether the double space is rendered at all? I think this could get collapsed into a single space by the browser? if it's rendered, fixing the doublespace without moving the null-check could work something like this: ... owner ${organizationOwnerName}to (no space before "to") and then organizationOwnerName should have a trailing space if it's not empty...

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I would not optimize for extremely unlikely case that an orga has no owner. This can only ever be the case if a DB migration was not executed correctly.

Copy link
Member

Choose a reason for hiding this comment

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

good point 👍

frontend/javascripts/components/pricing_enforcers.tsx Outdated Show resolved Hide resolved
Copy link
Member

@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.

Thanks for looking into this! I added some suggestions for the scala code :)

app/models/user/User.scala Outdated Show resolved Hide resolved
app/models/organization/OrganizationService.scala Outdated Show resolved Hide resolved
app/models/organization/OrganizationService.scala Outdated Show resolved Hide resolved
hotzenklotz and others added 8 commits February 7, 2023 13:44
…a_owner

* 'master' of github.com:scalableminds/webknossos:
  Fix error message when trying to join an orga you are already in (#6824)
  Access Google Cloud Storage via NIO (#6775)
… orga_owner

* 'orga_owner' of github.com:scalableminds/webknossos:
  Update app/models/user/User.scala
… orga_owner

* 'orga_owner' of github.com:scalableminds/webknossos:
  Update app/models/organization/OrganizationService.scala
…a_owner

* 'master' of github.com:scalableminds/webknossos:
  fixes loki log batching (#6828)
… orga_owner

* 'orga_owner' of github.com:scalableminds/webknossos:
Copy link
Member

@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.

Backend LGTM. Did not test, though

@philippotto
Copy link
Member

philippotto commented Feb 8, 2023

I noticed two things during testing:

  • the tooltip says "Ask the owner (...)" even when the current user is the owner actually. I think this is weird and we should try to avoid this wording in that case.
  • [ ] in case of the proofreading tool, there is no popover but still the old tooltip. is this intended?

Other than that, it works great 👍

@hotzenklotz
Copy link
Member Author

in case of the proofreading tool, there is no popover but still the old tooltip. is this intended?

Yes, this is intended. The toolbar has uses a tooltip for the description of all the individual tools that was "hijacked" for this message. This keeps things simple.

@philippotto
Copy link
Member

in case of the proofreading tool, there is no popover but still the old tooltip. is this intended?

Yes, this is intended. The toolbar has uses a tooltip for the description of all the individual tools that was "hijacked" for this message. This keeps things simple.

Ok, fair enough 👍

…a_owner

* 'master' of github.com:scalableminds/webknossos:
  Loki batched insert (#6831)
  Fix editable text style (#6799)
  Fix dbtool schema scheck stderr capturing (#6830)
  Release 23.02.1 (#6827)
… orga_owner

* 'orga_owner' of github.com:scalableminds/webknossos:
@@ -236,6 +242,7 @@ export function getDisabledInfoForTools(state: OxalisState): Record<
hasAgglomerateMappings,
genericDisabledExplanation,
state.activeOrganization,
enforceActiveUser(state.activeUser),
Copy link
Member

Choose a reason for hiding this comment

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

why enforce? getFeatureNotAvailableInPlanMessage should be able to handle null for the active user. i think this could crash currently when viewing a public annotation.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Great 👍

@hotzenklotz hotzenklotz merged commit b3673cd into master Feb 8, 2023
@hotzenklotz hotzenklotz deleted the orga_owner branch February 8, 2023 16:12
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.

3 participants