[Importer] Adding an option to copy and not just move files #3520
Conversation
agl29
requested review from
wing2fly,
JohanAhlen,
bjornalm,
ranade1,
athithyaaselvam and
Harshg999
| @@ -2633,6 +2640,7 @@ ${ commonheader(_("Importer"), "indexer", user, request, "60px") | n,unicode } | |||
| self.isTransactionalVisible = ko.observable((vm.sourceType == 'impala' && isTransactionalVisibleImpala) || (vm.sourceType == 'hive' && isTransactionalVisibleHive)); | |||
| self.isTransactional = ko.observable(self.isTransactionalVisible()); | |||
| self.isTransactional.subscribe(function(val) { | |||
| self.copyFile(false); | |||
There was a problem hiding this comment.
This sets self.copyFile to false regardless of what the isTransactional value is changed to, is that correct? I would assume you only want to set the self.copyFile to false if isTransactional is true, given the visibility logic of the input.
There was a problem hiding this comment.
Option of transactional comes first then copy file option will come, hence I thought user must choose the option of copy file after he/she decided that table should be transactional or not. So if he/she wanted to create transactional table then we are hiding the option of copy file (as this condition is not possible) and setting it to 'False' and if user disabled the transactional option then we are providing the option of copy file, and user can choose whether he/she wants to copy the file or not.
There was a problem hiding this comment.
Ok, so in practice it doesn't really matter that we set self.copyFile to false even when the isTransactional is set to false.
There was a problem hiding this comment.
Thinking more about this, this UI flow is a bit strange and could lead to mistakes, e.g:
- User checks Copy file
- User checks Transactional table
- User unchecks Transactional table (perhaps it was a misstake to select it in the first place)
- Now the Copy file selection has been undone and the user might miss that when moving forward.
Do we really need to change the state of the "Copy file" checkbox if it is not going to be used? Can't we just set a default value of false in the data we are submitting.
There was a problem hiding this comment.
I agree Bjorm's thinking the logic in the UI flow of when to use copy option.
When there is a logic in UI, always put a validation in the backend code also to enforce the consistency. Also, do we expose importer in pubic API?
agl29
force-pushed
the
importer_copy_file_option
branch
3 times, most recently
from
82b7024
to
461bd02
Compare
agl29
force-pushed
the
importer_copy_file_option
branch
3 times, most recently
from
115f487
to
f50c676
Compare
agl29
force-pushed
the
importer_copy_file_option
branch
from
f50c676
to
a21bf47
Compare
(cherry picked from commit 82fd88d)
What changes were proposed in this pull request?
How was this patch tested?