Put trusted notebooks behind an experiment #12712
Conversation
…nto trusted-notebooks-experiment
joyceerhl
requested review from
DavidKutu,
DonJayamanne,
IanMatthewHuff and
rchiodo
…nto trusted-notebooks-experiment
|
Oops. @rchiodo @IanMatthewHuff @DavidKutu I just set up the experiment in Control Tower and tested locally, and the code we currently have will show the 'Trusted' message for all users who are not enrolled in the experiment. Should we show the user anything related to trusted notebooks at all if the feature is turned off? |
|
No it should show nothing until the experiment is activated. |
|
|
||
| @injectable() | ||
| export class TrustService implements ITrustService { | ||
| public get experimentEnabled() { |
There was a problem hiding this comment.
This feels like your overcomplicating stuff for the experiment. I think it would be better to put all the interfaces back the way they were, and just pass a flag to the UI on whether or not to show the 'trust' part. We do this for the 'useCustomEditor' stuff too.
There was a problem hiding this comment.
Yeah agreed, the only reason I didn't do that to start with was that we'd need to send that shouldShowTrustMessage flag over with the cells, which means updating all the redux stuff. I think it ends up being the same number of files that need to be changed.
Also, this experimentEnabled stuff is inside trustService because I didn't want nativeEditor.ts to have to query the exp service to get the value of the shouldShowTrustMessage flag and send it over. My thought was that whether the experiment is enabled is a property of the trustService class, so we should read it off that. But I can see that this exposes more stuff on the public interface, so I'm happy to drop these changes.
There was a problem hiding this comment.
The implicit 'undefined' meaning not enabled was really my biggest concern. That was a new implicit idea in the code that isn't easy to understand.
TrustService exposing experiment enabled was less smelly IMO but still seemed off. There's an experiment service. Why would the TrustService expose something to do with experiments? Your then exposing the concept of an experiment where none is needed.
The UI does need to know about the experiment though, so somebody has to tell it. I think it makes sense for the nativeEditor.ts class to do that. It controls the UI and would make other decisions based on experiments to tell the UI.
There was a problem hiding this comment.
I agree, also not sure about the tri state of trusted using that to determine whether trust=undefined means not in experiment.
I'd just default to true everywhere and not display the UI as @rchiodo has suggested.
There was a problem hiding this comment.
Yeah it ended up being the exact same number of files changed (14) but what I had before was definitely harder to follow. Thanks guys 😊
| setSharedProperty('ds_notebookeditor', e?.type); | ||
| this.nativeContext.set(!!e).ignoreErrors(); | ||
| this.isNotebookTrusted.set(e?.model?.isTrusted === true).ignoreErrors(); | ||
| this.isNotebookTrusted.set(e?.model?.isTrusted !== false).ignoreErrors(); |
There was a problem hiding this comment.
| this.isNotebookTrusted.set(e?.model?.isTrusted !== false).ignoreErrors(); | |
| this.isNotebookTrusted.set(e?.model?.isTrusted).ignoreErrors(); |
Isn't this the same?
There was a problem hiding this comment.
Else the context variable gets set to true, which would be incorrect based on your comments.
I.e.
- trusted = true (then yes)
- trusted = false (then no)
- trusted = undefined (then not used).
Shouldn't the same logic apply to this context variable?
There was a problem hiding this comment.
If trusted=undefined we want to show all commands for editing notebooks, so the context variable should be set to true. But I agree with Rich and I'm reverting these changes, will check in new code shortly
|
|
||
| @injectable() | ||
| export class TrustService implements ITrustService { | ||
| public get experimentEnabled() { |
There was a problem hiding this comment.
I agree, also not sure about the tri state of trusted using that to determine whether trust=undefined means not in experiment.
I'd just default to true everywhere and not display the UI as @rchiodo has suggested.
|
Kudos, SonarCloud Quality Gate passed!
|
5 participants
For #12146
Note that we still need to enable this experiment in Control Tower before this PR can be merged.
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).