Removed eula popup notification #2512
Conversation
|
I think we need to leverage api versioning for the endpoint removal, right @arbulu89 @CDimonaco ? |
There was a problem hiding this comment.
There is some extra eula related frontend items that might be removed.
Besides this GET /api/v1/settings needs to be bumped to v2.
Not sure if the removal of the other two APIs is breaking BC (guess so)
==========================================================================
== API CHANGE LOG ==
==========================================================================
Trento
--------------------------------------------------------------------------
-- What's Deleted --
--------------------------------------------------------------------------
- POST /api/v1/accept_eula
- POST /api/v1/settings/accept_eula
--------------------------------------------------------------------------
-- What's Changed --
--------------------------------------------------------------------------
- GET /api/v1/settings
Return Type:
- Changed 200 OK
Media types:
- Changed application/json
Schema: Broken compatibility
Missing property: eula_accepted (boolean)
--------------------------------------------------------------------------
-- Result --
--------------------------------------------------------------------------
API changes broke backward compatibility
--------------------------------------------------------------------------
There was a problem hiding this comment.
Hey @chrisb1s ,
The removal looks good so far. Some comments from my side
I'm not that sure if we can just remove a endpoint without bumping the controller or schemas version.
We can take that risk, but users using it might start getting failures.
I guess we need to discuss that.
|
|
||
| def change do | ||
| alter table(:settings) do | ||
| remove :installation_settings_eula_accepted |
There was a problem hiding this comment.
@CDimonaco doing this is safe in our new STI design, right? Just double-checking
| @@ -0,0 +1,9 @@ | |||
| defmodule Trento.Repo.Migrations.DropColumnInstallationSettingsEulaAccepted do | |||
There was a problem hiding this comment.
I wouldn't call this Drop..., i would call Remove... as we do with every other remove migration
lib/trento/settings.ex
Outdated
| @@ -26,6 +26,10 @@ defmodule Trento.Settings do | |||
| installation_id | |||
| end | |||
|
|
|||
| def accept_eula do | |||
There was a problem hiding this comment.
We can safely remove this function.
| @@ -21,11 +21,29 @@ defmodule TrentoWeb.V1.SettingsController do | |||
| def settings(conn, _) do | |||
| render(conn, "settings.json", | |||
| settings: %{ | |||
| eula_accepted: true, | |||
There was a problem hiding this comment.
Let's better return false
|
|
||
| @spec accept_eula(Plug.Conn.t(), any) :: Plug.Conn.t() | ||
| def accept_eula(conn, _) do | ||
| :ok = Settings.accept_eula() |
There was a problem hiding this comment.
We don't need to run this dummy function. Just return the empty json
| @@ -3,12 +3,10 @@ defmodule TrentoWeb.V1.SettingsView do | |||
|
|
|||
| def render("settings.json", %{ | |||
| settings: %{ | |||
| eula_accepted: eula_accepted, | |||
There was a problem hiding this comment.
We cannot remove this 2 lines. It will just return what the controller returns, a hardcoded false
arbulu89
added
removal
env
Description
Completely removed the Eula popup message from the web app. This includes:
How was this tested?
Tested with the tests provided by the module/tests.