Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Update NPS Value Survey #9638
feat: Update NPS Value Survey #9638
Changes from 13 commits
c5923c3
931ab36
6295b8d
a6fdb1e
213fb7a
c6e195c
0c4a75e
1858e28
08acc4e
e55a241
4a4b9fa
2a8ce40
b7acea8
7c0c2ff
33f5c46
9b31e25
995946e
7ef0d9d
e07e054
a560053
29b19f3
cc6b2fc
007cee7
0436667
e67caad
0aec1b4
135737e
efc7c74
a48601d
81a29fc
3841598
6e90a25
9cfef22
61d2ba0
c333361
87a14ef
10bdd1c
2d5963e
aaf4aa3
52b4ad0
26db1df
0854acd
fcd5998
1403e5e
aa71cc8
cd1224a
f846d65
4a5ee8f
14fa060
dc7ff61
3200e7e
7a3a95b
22ba1d1
8058b39
8ab51ca
ff9218f
c50b9ba
ebc1e86
9ec6414
0be328e
b60e4cd
683848a
962a3e8
e84151d
04db697
51f58b4
aa72281
d4d6829
dd03c94
f517535
d8a9b47
9fb993d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
test failing on master. @MiloradFilipovic is looking into this. https://github.com/n8n-io/n8n/actions/runs/9453100846/job/26037911945
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.
Please ensure that this doesn't get left in the end
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.
Do we even want to fetch the prompts data if
currentUserId
is not set? Why not skip the whole requests then? In what case might this happen? Is there a possible race condition that this is called beforecurrentUserId
has value?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.
Sure, I can add this. Technically no, since specific user actions are needed to trigger this, for example save a workflow.
I think this logic predates user management so maybe that's why. We did not have user id, just instance 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.
for a long time in n8n until v1, setting up an owner user was optional, even after we added user management. so userId could be undefined but a user is logged in.
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.
If it should always be defined, it would be nicer to have an
assert
to satisfy TS compiler and to communicate that this should always be defined when this gets called. Otherwise it can cause nasty bugs that are hard to debug (e.g. if there's a bug that this function gets always called before thecurrentUserId
is defined the nps survey will never get initialized properly).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.
Yes that makes sense. Thanks for the suggestion
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.
What happens if this fails?
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.
nothing, but maybe the nps survey will show. n8n api might be down, but it should not affect UX and no need to show notification (we make this call too often and user does not know about it).