[FIX: #39152] Public links refer to null when question is new
#39154
Conversation
WiNloSt
added
backport
| if (t && resourceType === "question" && !resource.isSaved()) { | ||
| onModalOpen(MODAL_TYPES.SAVE_QUESTION_BEFORE_EMBED); |
There was a problem hiding this comment.
I don't like this solution. It makes this component tightly coupled with how it should be used, ultimately, in ViewFooter or DashboardEmbedAction and this makes the onModalOpen API inconsistent which creates confusion.
If you look in ViewFooter (query builder)
onModalOpen = (modalMode) => void
but in DashboardEmbedAction
onModalOpen = (void) => void
Also, onMenuToggle here conditonally calls onModalOpen which makes it hard to understand when my onModalOpen would be called or not.
Could you simplify this part more? I'd expect the onModalOpen API to be more consistent, and don't have this kind of condition inside the button. The components that use EmbedMenu should be the one that tells which modal to open instead of doing that in this component
There was a problem hiding this comment.
Yep, I felt uncomfortable writing this solution as well. You make a good point about the consistency of the onModalOpen call, I'll see if I can make it consistent across the different components
| } | ||
| /> | ||
| ), | ||
| type === "question" && |
There was a problem hiding this comment.
I moved the question.isSaved() logic up here. It almost mimics DashboardEmbedAction where it gatekeeps the actual menu until the question is saved.
There was a problem hiding this comment.
I like this. It looks more consistent than the previous version. It's just that now we're using DashboardEmbedHeaderButton inside query_builder/ directory which is for questions.
If this isn't specifically for dashboard anymore we should rename it accordingly. Like to just ResourceEmbedHeaderButton or something.
There was a problem hiding this comment.
Will change right now. Good call
There was a problem hiding this comment.
@WiNloSt let me know what you think. I renamed DashboardHeaderEmbedButton to ResourceEmbedButton (in dashboards, the button is in the header, but in questions it's in the footer so "Header" is not the greatest name).
Also I moved the component into the public/components folder, together with the EmbedModal and EmbedFrame components which the button leads to in the user flow.
| @@ -817,6 +840,27 @@ function enableEmbeddingForResource({ resource, id }) { | |||
| }); | |||
| } | |||
|
|
|||
| function assertValidPublicLink({ resource, shouldHaveRemoveLink = true }) { | |||
There was a problem hiding this comment.
Let me know if there's an easier way to do this. I've added the regex because it will help us make sure that we're not getting any null values in tests
There was a problem hiding this comment.
Obviously, that shouldn't be an issue in the future but I like it as an extra layer of protection in case anything gets weird
There was a problem hiding this comment.
Fair enough. Even though this looks a bit ugly, it gives me confident that the public link gotta be in a certain format, which is better than just asserting it's not null or empty.
There was a problem hiding this comment.
I'd suggest making shouldHaveRemoveLink required, this will force you to pass shouldHaveRemoveLink in every assertion and make the assertions clearer.
It's much more clear because you don't need to assume there would be default assertions somewhere else.
e.g. between
assertValidPublicLink({ resource: "question" });VS
assertValidPublicLink({
resource,
shouldHaveRemoveLink: true,
});It isn't that apparent here, but when you have a bunch of optional flags, when reading the test you gotta always think about how do I interpret this tests, what optional flags do I need to take into account, which could make reading the tests a lot harder.
|
| @@ -817,6 +840,27 @@ function enableEmbeddingForResource({ resource, id }) { | |||
| }); | |||
| } | |||
|
|
|||
| function assertValidPublicLink({ resource, shouldHaveRemoveLink = true }) { | |||
There was a problem hiding this comment.
Fair enough. Even though this looks a bit ugly, it gives me confident that the public link gotta be in a certain format, which is better than just asserting it's not null or empty.
| @@ -817,6 +840,27 @@ function enableEmbeddingForResource({ resource, id }) { | |||
| }); | |||
| } | |||
|
|
|||
| function assertValidPublicLink({ resource, shouldHaveRemoveLink = true }) { | |||
There was a problem hiding this comment.
I'd suggest making shouldHaveRemoveLink required, this will force you to pass shouldHaveRemoveLink in every assertion and make the assertions clearer.
It's much more clear because you don't need to assume there would be default assertions somewhere else.
e.g. between
assertValidPublicLink({ resource: "question" });VS
assertValidPublicLink({
resource,
shouldHaveRemoveLink: true,
});It isn't that apparent here, but when you have a bunch of optional flags, when reading the test you gotta always think about how do I interpret this tests, what optional flags do I need to take into account, which could make reading the tests a lot harder.
oisincoveney
force-pushed
the
39152-public-links-are-null-for-new-questions
branch
from
cca332d
to
dd6916e
Compare
Closes #39152
Description
When questions are new, they don't have an ID and aren't able to possess a public_uuid since they aren't saved. To solve this, we need to make sure that the user has saved the question before they try to create a public link for it.
How to verify
First, save your questionmodal pops up first.Demo
Upload a demo video or before/after screenshots if sensible or remove the section
Checklist