Include table visualization in pulses and alerts #7022
Conversation
senior
force-pushed
the
pulse-tables
branch
5 times, most recently
from
76931e1
to
993135d
Compare
|
|
wrt point 3, should those even be in the rendered table? We have a field visibility of |
|
The issue with #2 above is we're exceeding the file size limit allowed by SES. It's returning an error message (that isn't being bubbled up) from the email sending code. We should limit the number of results attached to 2000, just like we do for table displays today. We should also bubble up that message sending error to the UI. |
|
@senior and I found that our SMTP server has a 10MB limit for emails, which was causing pulses with super large CSV attachments to choke and not send. Consequently, we're proposing that we limit CSV attachments to 2,000 rows. This would make it far less likely that we'd hit SMTP size limits, and would be consistent with the row limit we have in-app. This is a much smaller limit than our downloads limit, but we're thinking that if you're sending tables with more than 2,000 rows in the full result, you're not really using Pulses for their intended use, and you should instead download the full large results and email them manually. Opinions, @salsakran? |
|
My opinion is that there's a special place in hell for people who distribute huge files via SMTP. Imo anything that big should just be a download link. That said, I don't like limits without being aware of them however. Eg, how does the user know that the csv they got has 2000 or 2000 (out of 10,000) rows? |
Yep, I'll need to come up with a solution for this question. |
|
There's another factor -- different setups (read: companies) have different limits here. |
|
Yeah, we talked about that, and that was another reason for choosing a relatively conservative limit, since we can't really know what each instance's SMTP server's limit is going to be. |
|
@senior, update on item #5 in my comment above: it looks like the card preview code is having problems with GUI-built pivot tables, too. When I try to add this question, I get this error:
Interestingly, it's not having problems with this other GUI pivot table, and the difference appears to be that this one has several metrics/aggregations, but only a single breakout field; the question above had two breakout dimensions. My hunch is that this has to do with the wacky logic in Pulses that was previously used to determine if we should show a small table, or if we should error out and say "you can't include raw data questions" in the UI.
Here it is in the Pulse preview UI:
|
|
@salsakran @mazameli I am working on hiding fields with a visibility type of |
|
@senior yes. |
|
@mazameli I've implemented that UI. @senior It seems like there's some unrelated integration tests failing and I'm not sure why. In particular |
|
This is ready to go from my perspective once poor @senior figures out what's up with the FE tests. |
|
@salsakran @tlrobinson Not sure if I'm on the right track or not. The error mentioned here is related to an error sending an email. Previously that error was logged, then swallowed, returning a 200. Now it will generate a correct error code and the front end correctly lets the user know. I just pushed a commit that adds a dev dependency on maildev and adds the appropriate settings to the H2 database that the tests use. Only problem is that test still isn't working and it broke two other tests that I think were relying on there not being an smtp host configured. Below is what I see now locally. |
Previously pulses and alerts that used a table visualization were limited to 3 columns and 10 rows. This commit changes the backed to bump that to 10 columns and 20 rows. When a resultset has more data than will fit into a 10x20 table, it will trigger a CSV attachment that will include the full results of the query, up to 2000 results. This commit also limits all queries to 2000 rows, similar to how queries issued via the UI are constrained. Users wanting more than 2000 rows use the existing export functionality.
|
to limit to 2000 rows for xlsx file, is too limited. |
|
Fixes #7001 |
I agree with this too |
If the query result is a table visualization show the first 20 rows
and 10 columns. If there are more than 20 rows or 10 columns, include
a CSV that has the full resultset.
Backend TODO
Frontend TODO
PulseEditCards.jsx, add logic to show a notice/warning when a table is larger than either 20 rows or 10 columns to say, "[head]Heads up[\head] [body]We'll display part of this table in your Pulse, and add a file attachment with more results, up to 2,000 rows."PulseEditCards.jsxin this state the UI should show the file attachment options automatically for this card, and disable or hide the paperclip icon (to make it clear that the file attachment is going to happen automatically for this question).Design/CSS to-dos: