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.

Sign up for GitHub

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

Restrict teams features based on org status #2897

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Restrict teams features based on org status #2897

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac96e4b
Restrict teams features based on org status
wojtekmach Nov 29, 2024
a6aad6a
Add `@disabled` to `LivebookWeb.Hub.FileSystemFormComponent`
wojtekmach Dec 17, 2024
c324d38
Remove some duplication
wojtekmach Dec 18, 2024
05045c7
Rename active to disabled
wojtekmach Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/livebook/hubs/team.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ defmodule Livebook.Hubs.Team do
field :session_token, :string, redact: true
field :hub_name, :string
field :hub_emoji, :string
field :active, :boolean, default: true

embeds_one :offline, Offline
end
Expand Down
25 changes: 21 additions & 4 deletions lib/livebook/hubs/team_client.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,7 @@ defmodule Livebook.Hubs.TeamClient do

defp handle_event(:user_connected, user_connected, state) do
state
|> update_hub(user_connected)
|> dispatch_secrets(user_connected)
|> dispatch_file_systems(user_connected)
|> dispatch_deployment_groups(user_connected)
Expand Down Expand Up @@ -728,6 +729,10 @@ defmodule Livebook.Hubs.TeamClient do
state
end

defp handle_event(:org_updated, org_updated, state) do
update_hub(state, org_updated)
end

defp dispatch_secrets(state, %{secrets: secrets}) do
decrypted_secrets = Enum.map(secrets, &build_secret(state, &1))

Expand Down Expand Up @@ -796,14 +801,26 @@ defmodule Livebook.Hubs.TeamClient do
state
end

defp update_hub(state, %{public_key: org_public_key}) do
hub = %{state.hub | org_public_key: org_public_key}
defp update_hub(state, %LivebookProto.UserConnected{org_active: active}) do
update_hub(state, &put_in(&1.active, active))
end

defp update_hub(state, %LivebookProto.OrgUpdated{active: active}) do
update_hub(state, &put_in(&1.active, active))
end

defp update_hub(state, %LivebookProto.AgentConnected{public_key: org_public_key}) do
update_hub(state, &put_in(&1.org_public_key, org_public_key))
end

defp update_hub(state, fun) when is_function(fun, 1) do
hub = fun.(state.hub)

if Livebook.Hubs.hub_exists?(hub.id) do
if Hubs.hub_exists?(hub.id) do
Hubs.save_hub(hub)
end

%{state | hub: hub}
put_in(state.hub, hub)
end

defp diff(old_list, new_list, fun, deleted_fun \\ nil, updated_fun \\ nil) do
Expand Down
3 changes: 3 additions & 0 deletions lib/livebook_web/live/hub/edit/personal_component.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ defmodule LivebookWeb.Hub.Edit.PersonalComponent do
id="hub-file-systems-list"
hub_id={@hub.id}
file_systems={@file_systems}
disabled={false}
/>
</div>

Expand Down Expand Up @@ -195,6 +196,7 @@ defmodule LivebookWeb.Hub.Edit.PersonalComponent do
hub={@hub}
secret_name={@secret_name}
secret_value={@secret_value}
disabled={false}
return_to={~p"/hub/#{@hub.id}"}
/>
</.modal>
Expand All @@ -210,6 +212,7 @@ defmodule LivebookWeb.Hub.Edit.PersonalComponent do
module={LivebookWeb.Hub.FileSystemFormComponent}
id="file-systems"
hub={@hub}
disabled={false}
file_system={@file_system}
file_system_id={@file_system_id}
return_to={~p"/hub/#{@hub.id}"}
Expand Down
25 changes: 23 additions & 2 deletions lib/livebook_web/live/hub/edit/team_component.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ defmodule LivebookWeb.Hub.Edit.TeamComponent do
{Provider.connection_status(@hub)}
</LayoutComponents.topbar>

<LayoutComponents.topbar :if={[email protected]} variant="warning">
<h2>
Workspace disabled: your organization doesn't have an active subscription. Please contact your <.link
href={org_url(@hub, "/users")}
class="underline"
>org's admin</.link>.
</h2>
</LayoutComponents.topbar>

<div class="p-4 md:px-12 md:py-7 max-w-screen-md mx-auto">
<div id={"#{@id}-component"}>
<div class="mb-8 flex flex-col space-y-2">
Expand Down Expand Up @@ -176,10 +185,15 @@ defmodule LivebookWeb.Hub.Edit.TeamComponent do
secrets={@secrets}
edit_path={"hub/#{@hub.id}/secrets/edit"}
return_to={~p"/hub/#{@hub.id}"}
disabled={not @hub.active}
/>

<div>
<.button patch={~p"/hub/#{@hub.id}/secrets/new"} id="add-secret">
<.button
patch={~p"/hub/#{@hub.id}/secrets/new"}
id="add-secret"
disabled={not @hub.active}
>
Add secret
</.button>
</div>
Expand All @@ -200,6 +214,7 @@ defmodule LivebookWeb.Hub.Edit.TeamComponent do
hub_id={@hub.id}
file_systems={@file_systems}
target={@myself}
disabled={not @hub.active}
/>
</div>

Expand Down Expand Up @@ -233,7 +248,11 @@ defmodule LivebookWeb.Hub.Edit.TeamComponent do
</div>

<div>
<.button patch={~p"/hub/#{@hub.id}/groups/new"} id="add-deployment-group">
<.button
patch={~p"/hub/#{@hub.id}/groups/new"}
id="add-deployment-group"
disabled={not @hub.active}
>
Add deployment group
</.button>
</div>
Expand Down Expand Up @@ -289,6 +308,7 @@ defmodule LivebookWeb.Hub.Edit.TeamComponent do
secret_name={@secret_name}
secret_value={@secret_value}
return_to={~p"/hub/#{@hub.id}"}
disabled={not @hub.active}
/>
</.modal>

Expand All @@ -303,6 +323,7 @@ defmodule LivebookWeb.Hub.Edit.TeamComponent do
module={LivebookWeb.Hub.FileSystemFormComponent}
id="file-systems"
hub={@hub}
disabled={not @hub.active}
file_system={@file_system}
file_system_id={@file_system_id}
return_to={~p"/hub/#{@hub.id}"}
Expand Down
5 changes: 5 additions & 0 deletions lib/livebook_web/live/hub/edit_live.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ defmodule LivebookWeb.Hub.EditLive do
def mount(_params, _session, socket) do
if connected?(socket) do
Hubs.Broadcasts.subscribe([:connection])

Livebook.Teams.Broadcasts.subscribe([:deployment_groups, :app_deployments, :agents])
end

Expand Down Expand Up @@ -111,6 +112,10 @@ defmodule LivebookWeb.Hub.EditLive do
{:noreply, load_hub(socket, id)}
end

def handle_info({:hub_changed, id}, %{assigns: %{hub: %{id: id}}} = socket) do
{:noreply, load_hub(socket, id)}
end

def handle_info(_message, socket) do
{:noreply, socket}
end
Expand Down
2 changes: 1 addition & 1 deletion lib/livebook_web/live/hub/file_system_form_component.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ defmodule LivebookWeb.Hub.FileSystemFormComponent do
</p>
<% end %>
<div class="flex space-x-2">
<.button type="submit" disabled={not @changeset.valid?}>
<.button type="submit" disabled={@disabled or not @changeset.valid?}>
<.remix_icon icon={@button.icon} />
<span class="font-normal">{@button.label}</span>
</.button>
Expand Down
6 changes: 5 additions & 1 deletion lib/livebook_web/live/hub/file_system_list_component.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ defmodule LivebookWeb.Hub.FileSystemListComponent do
</div>
</div>
<div class="flex">
<.button patch={~p"/hub/#{@hub_id}/file-systems/new"} id="add-file-system">
<.button
patch={~p"/hub/#{@hub_id}/file-systems/new"}
id="add-file-system"
disabled={@disabled}
>
Add file storage
</.button>
</div>
Expand Down
2 changes: 1 addition & 1 deletion lib/livebook_web/live/hub/secret_form_component.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ defmodule LivebookWeb.Hub.SecretFormComponent do
<.hidden_field field={f[:hub_id]} value={@hub.id} />
<.hidden_field field={f[:deployment_group_id]} value={@deployment_group_id} />
<div class="flex space-x-2">
<.button type="submit" disabled={not @changeset.valid?}>
<.button type="submit" disabled={not @changeset.valid? or @disabled}>
<.remix_icon icon={@button.icon} />
<span class="font-normal">{@button.label}</span>
</.button>
Expand Down
1 change: 1 addition & 0 deletions proto/lib/livebook_proto/agent_connected.pb.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ defmodule LivebookProto.AgentConnected do
json_name: "appDeployments"

field :agents, 9, repeated: true, type: LivebookProto.Agent
field :org_active, 10, type: :bool, json_name: "orgActive"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, we have a dependency issue here. We need to either:

  1. Deploy Teams with this before we publish a new Livebook version
  2. Rename field to org_disabled : bool or something like org_status : active | disabled

The reason is the default value for booleans is false so if Teams is not sending this value it is considered false which would block Teams features. The DSL allows a default: true, which would be my preference, but that is not available on proto3.

I guess option 1 is fine, option 2 with :org_status could be more future proof though. @jonatanklosko @josevalim @hugobarauna Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Making it an enum is probably going to be better in the long term?

Copy link
Member

Choose a reason for hiding this comment

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

@josevalim

livebook/proto/messages.proto

Lines 164 to 174 in 04a93c0

/**
* We're only using Enum in this case because
* the Client is the source of the information,
* and the Server will always be up-to-date.
*
* Otherwise, it shouldn't be used.
*/
enum AppDeploymentStatusType {
preparing = 0;
available = 1;
}

Copy link
Member

@jonatanklosko jonatanklosko Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We are going to deploy Teams first anyway (1.), otherwise the new Livebook release would probably have issues, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, correct. We always need to deploy Livebook Teams first before we release a new Livebook. But if anyone uses main or nightly build, then it will be fully broken saying they need to pay. That's why we may also want to have an enum with better defaults? Or the enum would be broken as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why couldn't we return more values in the future? Is that Protobuf limitation or it's depend on us how we treat incoming values (e.g. exhaustive case/2 on exact values)?

Copy link
Contributor

Choose a reason for hiding this comment

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

So let's ship with the inverse for now: inactive

Copy link
Member

Choose a reason for hiding this comment

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

Why couldn't we return more values in the future? Is that Protobuf limitation or it's depend on us how we treat incoming values (e.g. exhaustive case/2 on exact values)?

The protobuf library would keep an unknown value, but it would be an integer (see commit message in elixir-protobuf/protobuf@6b8f40f). Leaking the integer to the rest of the application is probably a bad idea, so we would need to account for the unknown value early. In some cases this could be as simple as picking a default value, but here I'm not sure if that's clear. Using strings has the benefit that it's safer to keep them around, but it mostly pushes the catch all and default behaviour problem to other parts of the app.

I don't remember if there was a specific scenario behind the AppDeploymentStatusType comment though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im gonna go with inactive bool flag then. Though if anyone prefers can call it disabled, lmk. Thanks everyone!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

end
2 changes: 2 additions & 0 deletions proto/lib/livebook_proto/event.pb.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,6 @@ defmodule LivebookProto.Event do
type: LivebookProto.AppDeploymentStopped,
json_name: "appDeploymentStopped",
oneof: 0

field :org_updated, 17, type: LivebookProto.OrgUpdated, json_name: "orgUpdated", oneof: 0
end
6 changes: 6 additions & 0 deletions proto/lib/livebook_proto/org_updated.pb.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
defmodule LivebookProto.OrgUpdated do
use Protobuf, protoc_gen_elixir_version: "0.13.0", syntax: :proto3

field :id, 1, type: :string
field :active, 2, type: :bool
end
1 change: 1 addition & 0 deletions proto/lib/livebook_proto/user_connected.pb.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ defmodule LivebookProto.UserConnected do
json_name: "appDeployments"

field :agents, 6, repeated: true, type: LivebookProto.Agent
field :org_active, 7, type: :bool, json_name: "orgActive"
end
8 changes: 8 additions & 0 deletions proto/messages.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ message UserConnected {
repeated DeploymentGroup deployment_groups = 4;
repeated AppDeployment app_deployments = 5;
repeated Agent agents = 6;
bool org_active = 7;
}

message AgentConnected {
Expand All @@ -118,6 +119,7 @@ message AgentConnected {
repeated DeploymentGroup deployment_groups = 7;
repeated AppDeployment app_deployments = 8;
repeated Agent agents = 9;
bool org_active = 10;
}

message AppDeployment {
Expand Down Expand Up @@ -154,6 +156,11 @@ message AgentLeft {
string id = 1;
}

message OrgUpdated {
string id = 1;
bool active = 2;
}

message Agent {
string id = 1;
string name = 2;
Expand Down Expand Up @@ -207,5 +214,6 @@ message Event {
AgentJoined agent_joined = 14;
AgentLeft agent_left = 15;
AppDeploymentStopped app_deployment_stopped = 16;
OrgUpdated org_updated = 17;
}
}
Loading