-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow creating and editing limited views #5925
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
Conversation
|
- Update password UI to use a toogle switch to emphasize the optional nature of the feature - Add UI for limiting the link to a segment - Add toggle_field component for full-width label and toggle switch layout - Add optional link to the input component, showing below the input, which can be used for documentation links - Add eye icon to shared links list that indicates limited view - Add tooltips to icons in the shared links list
9383307 to
4a027b8
Compare
5ac232a to
befc3b7
Compare
sanne-san
left a comment
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.
🎉
|
There's some conflicts that need to be resolved |
aerosol
left a comment
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.
Comments inline
lib/plausible/segments/segments.ex
Outdated
| type = :site | ||
| fields = [:id, :name] | ||
|
|
||
| name_empty? = is_nil(name) or (is_binary(name) and String.trim(name) == "") |
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.
When do we expect this to be nil? How about call it input - following the call site - or even user_inputand assume binary in the function clause?
| fragment( | ||
| "CASE | ||
| WHEN lower(?) = lower(?) THEN 0 -- exact match | ||
| WHEN lower(?) LIKE lower(?) || '%' THEN 1 -- starts with |
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.
This is nice but a bit of an overkill, since there's only 4 customers with more than 20 site segments. I'd consider simplifying that query and using Enum.sort_by/3 combined with String.jaro_distance/2 to achieve similar effectiveness and arguably simpler code. Ignore if you wish.
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.
Thanks for looking those stats up! It is indeed overkill then for this particular combobox. I liked how it came out though. I suggest we keep it and introduce it to other comboboxes as well. Server side sorting means we should query for bigger pages - it'd be good to be able to avoid it.
| end | ||
| end | ||
|
|
||
| def get_related_shared_links(%Plug.Conn{} = conn, _params), do: invalid_request(conn) |
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.
Isn't that redundant? If no controller clause matched, phoenix should respond with a bad request, no?
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.
Checking, thought it responds with internal server error.
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.
Checked just now. If the endpoint handler doesn't have any matching function clauses, it returns an internal server error. These fallbacks (all of the segments endpoints have them) haven't been triggered lately, maybe never (checked some traces). Looks like our authorization plugs are pretty robust. Nothing in the FE depends on 400 over 500, so I could remove these clauses. It's just that all of these endpoints will be superfluous after LV dashboard anyway, so I don't see the point in perfecting them.
| if conn.assigns.shared_link && conn.assigns.shared_link.segment_id do | ||
| segment_id = conn.assigns.shared_link.segment_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.
Could match in the function clause instead. I feel the plug name could be better here, since it's more specific to segments than filters?
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.
Roger that on the match! Regarding the name, I specifically wanted it to be more general, so readers wouldn't need to be aware of the details of segments and limited views, just that some filters may be required.
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.
Refactored with function clauses, kept the name: 88a80a1
| end | ||
|
|
||
| defp ensure_expected_segment_filter_present( | ||
| %{"filters" => filters} = _params, |
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.
will users ever run into an error like that? Unclear why do we need to enforce a filter to be "first".
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.
Good question! They won't if they play by the rules. If they try to request stats with the wrong filters though, they'll get a bad request error. The alternative to having it first is allow it to be anywhere, which we wouldn't be able to match on. Limited view FE forces it to be first, so I thought we might as well make use of that. Will add a test for this error.
Matching on raw filters params before they go through Query parsing, as I do here, is not ideal though. If I were to match on filters after Query.from, I'd have to add this little function to every internal stats endpoint, so I went this way.
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.
Added test: 88a80a1
| current_user = conn.assigns[:current_user] | ||
| site_role = get_fallback_site_role(conn) | ||
| shared_link = Plausible.Repo.preload(shared_link, site: :owners) | ||
| shared_link = Plausible.Repo.preload(shared_link, site: [:owners], segment: []) |
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.
The intention behind preloading an empty list is unclear here. Don't understand what's going on. Did you mean :segment? Oh and you've got Repo aliased here.
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.
Thanks! The intent is to preload both owners and segment relation, and that's the syntax I got from Ecto docs. It works, but I'll check whether it can be simpler.
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.
It can be simpler, just that the keyword list within a keyword list needs to be last elem: ea458fa
| > | ||
| <PlausibleWeb.Components.Generic.toggle_field | ||
| id="limit-view" | ||
| id_suffix="" |
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.
That's the default, no?
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.
| > | ||
| <PlausibleWeb.Components.Generic.toggle_field | ||
| id="password-protect" | ||
| id_suffix="" |
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.
ditto, default, can we omit?
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.
| attr(:rest, :global) | ||
|
|
||
| def toggle_field(assigns) do | ||
| help_text_conditional = assigns[:help_text_conditional] || false |
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.
why do we need this if attr macro defines a default?
Wondering if we could simplify this. As a component user I'd have very hard time figuring out what do I need to pass to help_text_conditional, what does it do even, not to mention js_active_var. Not a fan of this interface.
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.
Agreed that the interface was a bit clunky and mysterious. How's this? 84e6a07
| data-is-consolidated-view={Jason.encode!(@consolidated_view?)} | ||
| data-consolidated-view-available={Jason.encode!(@consolidated_view_available?)} | ||
| data-team-identifier={@team_identifier} | ||
| data-limited-to-segment-id={Jason.encode!(@limited_to_segment_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.
Will injecting some random integer here make react request someone else's segment?
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.
Good question, the answer is no.
There's actually no GET /segments/:id or GET /segments endpoints for the FE to call.
There used to be at one point, but there aren't any more.
The way it works now is: We stuff all the site's segments into data-segments, parse it in React, and update that FE segments list whenever the user changes or deletes one. Or the list gets updated whenever the dashboard is refreshed. It's not a great system, but does the job. LV dashboard should allow us to make it right.
If this data-limited-to-segment-id is set, it's the ID of the single segment stuffed into data-segments. If there's no such segment in data-segments, it's treated as if it was null. Because it was a shared link and the stats BE still expects segment filter to be present, all the reports get errors, which visually looks like the endless loader.
aerosol
left a comment
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.
Need someone else to look at react stuff, elixir stuff is mostly nitpicking at this point and my browser crashes trying to render the diff so not worth it
ukutaht
left a comment
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.
Looks good
| <option :if={@prompt} value="">{@prompt}</option> | ||
| {Phoenix.HTML.Form.options_for_select(@options, @value)} | ||
| </select> | ||
| <div :if={@link != [] && Enum.empty?(@errors)} class="mt-1"> |
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.
Where is this used?
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.
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.

Changes
TODO
Tests
Changelog
Documentation
Dark mode