Few bugfixes #2320
Few bugfixes #2320
Conversation
kravets-levko
commented
Feb 15, 2018
kravets-levko
commented
- https://discuss.redash.io/t/v4-beta-public-sharing-blank/1533
- After generating "Invite user" link and "Reset password" link, UI showed relative link (only path fragment), and UI was inconsistent (one of them was shown in textarea and another - as link). Now it looks like this:
| @@ -27,9 +27,9 @@ const layouts = { | |||
| function selectLayout(route) { | |||
| let layout = layouts.default; | |||
| if (route.layout) { | |||
| layout = layouts[route.layout]; | |||
| layout = layouts[route.layout] || layouts.default; | |||
| } else if (!route.authenticated) { | ||
| layout = layout.defaultSignedOut; | ||
| layout = layouts.defaultSignedOut; |
client/app/services/utils.js
Outdated
| @@ -0,0 +1,18 @@ | |||
| function fullUrl($location, url) { | |||
| const scheme = $location.protocol().toLowerCase(); | |||
There was a problem hiding this comment.
Why not use window.location and drop the dependency on Angular?
redash/handlers/dashboards.py
Outdated
| return serializers.public_dashboard(dashboard) | ||
| return project( | ||
| dashboard.to_dict(with_widgets=True, user=self.current_user), | ||
| ('name', 'layout', 'dashboard_filters_enabled', 'updated_at', 'created_at', 'widgets') |
There was a problem hiding this comment.
The serializer does more than that: it removes fields we don't want to expose publicly (like the query text) and includes fields we usually don't like the query result.
There was a problem hiding this comment.
I can agree with you, but my opinion (based on my experience) is that such code is very error-prone - when database schema or data structures (like dashboard layout in this case) changes - such code most likely will not be updated (like in in this case :-) ). So, can we implement this in some different way? say, id field is sensitive (BTW - is it really sensitive?) - maybe it's simpler just omit few fields? WDYT?
There was a problem hiding this comment.
How is it different when using project? The call to project will not be updated the same way the serializer wasn't updated. :-)
Some context on serializers: the idea is to stop putting serialization logic in the models code (the to_dict methods). It's not a concern of the models and it makes code harder to write, as we can't assume state we usually have when we serialize data (like current_user).
The mistake here (aside from not having good UI tests that could've caught this) was to duplicate widgets layout logic in the public dashboards serializer. As you suggested we need to call the non public serializer (currently the Dashboard#to_dict method), and omit data from the returned object (along with adding the query result). But we need to do it in the serializer.
There was a problem hiding this comment.
I realized that we suggested the same thing. The only difference is that current implementation missing the update of the widgets' properties and the location of the code (directly in handler or in serializer).
There was a problem hiding this comment.
I actually don't clearly understand why we need that serializer at all. Let me explain.
I see two reasons why we may need to transform data for public dashboards:
- protect sensitive data - in this case, I think it's better not to pick "safe" fields (with
project), but remove "unsafe" ones withomit. both options are error-prone; but we also should remember that front-end code expects data in some format, and when some fields are missing - it may lead to strange bugs. - save some traffic via removing unused fields - I think it's not a reason at all - we will save, say, 500 bytes, or even 1000 bytes (but it should be really large dashboard), but will make our code potentially buggy.
I addition, one day we're going to merge dashboard and public dashboard code, and in this case we (probably) will need the same data format for both (#2076).
If I'm missing something - just explain me please. I really want to make this better :-)
P.S. The only this I missed (noticed while typing this comment) - I removed code that processed hidden text boxes. It definitely should be restored.
P.P.S. Can't find code that processes hidden text widgets at all. Will check more carefully when working on changes to this PR - it's possible that we have one more bug here
There was a problem hiding this comment.
The main reason for omitting the fields is security concerns, because of that I actually think that project is the right choice. While both are error prone, it's better to have a bug than a data leak.
As for why put this in a serializer function rather inline in this handler:
- Better separation of concerns.
- To be able to reuse the code (the way we serialize visualizations/query can be reused in embed API calls for a visualization).
Hidden text boxes: I think we can drop this code. It was added by someone to allow adding <script> tags into the dashboard. That's not functionality that we want to keep supporting.
There was a problem hiding this comment.
Okay, got it. I'll amend this commit - it will be easier to restore code. But GitHub should keep our discussion.
About hidden textboxes - should we also remove the checkbox (and related JS) from UI?
|
@arikfr Updated this PR: restored serializers (but fixed them), removed code related to hidden text widgets (that flag is not used anywhere) |
client/app/services/utils.js
Outdated
| } | ||
|
|
||
| export default function init(ngModule) { | ||
| ngModule.factory('Utils', () => ({ |
There was a problem hiding this comment.
We don't need Angular's DI here. We can just export absoluteUrl and then where we need to use it:
import { absoluteUrl } from '@/services/utils';There was a problem hiding this comment.
Great idea, it's definitely better 👍 Will do in few minutes
