feat: Screen Config Postgres Client APIs and Admin Logic - #3231
feat: Screen Config Postgres Client APIs and Admin Logic#3231robbie-sundstrom wants to merge 3 commits into
Conversation
robbie-sundstrom
changed the title
04ba74c to
6607c7f
Compare
| get "/screens_by_alert", ScreensByAlertController, :index | ||
| end | ||
|
|
||
| scope "/api", ScreensWeb do |
There was a problem hiding this comment.
Non-blocking nit - From the perspective of someone who is trying to hit these endpoints for the first time, they'd probably have to start digging through the code to figure out why they'd getting 401's back. It might be worth putting in the README that you need to set an authorization header in requests to these endpoints
| const confirmFn = async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const config = configRef.current.value; |
There was a problem hiding this comment.
No action required - Can we give onConfirm a type? It looks like it should be a React.RefObject<HTMLTextAreaElement | null> based on it's creation here, but here we treat it as if it'll never be null here. If it ever is null, this'll fail at runtime
| end) | ||
| end | ||
|
|
||
| @doc """ |
There was a problem hiding this comment.
For my own understanding, we have a mix of @doc comments and blocks of comments (e.g.). How do we determine what we want to doc with @doc vs not?
There was a problem hiding this comment.
I do need to be consistent with this 😅 I kind of like the @doc comments because of how they integrate with tools like ElixirLS in VSCode. But I do generally think the @doc comments are moreso for API consumers, which is why we generally don't have them throughout our codebase. I'll move towards consistency here, which I'd like to be @doc comments for this new file at least
There was a problem hiding this comment.
The Elixir convention as far as I've seen is @doc for public functions and # blocks for private functions, since private functions are treated as "hidden" anyway and there's no way to access docs attached to them beyond looking at the code. Technically the main purpose of @doc is in generating ExDocs which are made available on Hex for published libraries; in an application codebase, it's mostly useful for editor LS integration. I usually include them if there's something important for callers to know that isn't covered by the function name, argument names, and typespec, which is all also readily accessible via LSP. (This is not just "for API consumers" in the sense of "consumers of some API external to the whole application", but if you take an expansive view of an "API" as "the set of public functions exported by a module", then that is accurate!)
| @spec perform_deletes([String.t()]) :: :ok | {:error, any()} | ||
| defp perform_deletes(deletes) do | ||
| Enum.reduce_while(deletes, :ok, fn id, _acc -> | ||
| Repo.delete_all(from s in ScreenConfig, where: s.id == ^id) |
There was a problem hiding this comment.
Does this fn ever return {:error, any()}? I deleted {:error, any()} from the return type, and mix dialyzer passed. Maybe something to do with the reduce_while and delete_all I'm not seeing?
This may be tied to the first question, but for my own understanding, why do we need to use reduce_while here? Wouldn't Enum.each/2 suffice here?
There was a problem hiding this comment.
Enum.each did originally suffice here, since we're not short circuiting early like in update_to_postgres in the event that we do encounter an error with deletion. I'm making some changes to error handling based on your other comments to add more detailed error codes and details, so I have moved this back to a reduce_while
| """ | ||
| @spec commit_updates([%{:id => String.t(), :config => map()}], [String.t()]) :: | ||
| :ok | {:error, any()} | ||
| def commit_updates(updates, deletes \\ []) do |
There was a problem hiding this comment.
I'm not entirely sure we should combine updates + deletes in a single API call. Maybe I'm being too much of a purist/this reminds me a little bit of SOAP calls though. Is there an advantage of combining these other than one less round trip from the API to the server?
There was a problem hiding this comment.
This is just recreating existing behavior, which calls a single API point for both updates and deletes at the same time. I agree in principle though, and I could update this so that the frontend make two separate API calls
| def index(conn, _params) do | ||
| screen_configs = | ||
| ScreenConfigs.list_screen_configs() | ||
| |> Enum.map(fn screen_config -> |
There was a problem hiding this comment.
Do we need this Enum.map call? Does the ScreenConfig list returned by list_screen_configs() have everything we need here?
There was a problem hiding this comment.
It actually has more info than is needed, since it includes the timestamp fields (created_at, updated_at). Since you pointed this out, I do think we could filter these out at the data access layer though
26fc1a9 to
bc959bf
Compare
bc959bf to
cafbc44
Compare
3 participants
Asana task: Define Screen Config shared APIs
indexScreens Admin endpoint now calls a different function that handles the feature flag logicscreen_configsendpoint that handles modifying/deleting screen configs in JSON or Postgres based on the feature flagFrontend Changes
commitScreenConfigChanges.editor.tsxcalls this directlyinspector.tsxnow calls this within theAdminFormcomponent.