New step remove sftp file #952
Conversation
gbarat87
force-pushed
the
new-step-move-sftp
branch
3 times, most recently
from
4cdb88d to
69241a1
Compare
There was a problem hiding this comment.
Could this also have unit tests added? e.g. see https://github.com/catalyst/moodle-tool_dataflows/blob/MOODLE_405_STABLE/tests/tool_dataflows_sftp_test.php
Since there's no actual SFTP to test against, it might just end up being a test for 1. sanity check that step loads properly, and 2. testing that source is always a remote, and errors otherwise
|
|
||
|
|
There was a problem hiding this comment.
This has been removed
| if ($sourceisremote) { | ||
| $this->delete_from_remote($sftp, $sourcepath); | ||
| return $input; | ||
| } |
There was a problem hiding this comment.
If the source isn't a remote, it should throw an exception here. Because it's validated at form validation time, if this is not a remote here it means something is very wrong.
There was a problem hiding this comment.
An exception has been raised here
lang/en/tool_dataflows.php
Outdated
| @@ -158,6 +158,7 @@ | |||
| $string['step_name_connector_set_variable'] = 'Set variable'; | |||
| $string['step_name_connector_set_multiple_variables'] = 'Set multiple variables'; | |||
| $string['step_name_connector_sftp'] = 'SFTP file copy'; | |||
| $string['step_name_connector_sftp_delete_file'] = 'SFTP file to delete'; | |||
There was a problem hiding this comment.
Could this be SFTP delete file instead ?
There was a problem hiding this comment.
This has been rewritten
| $hasremote = $sourceremote || $targetremote; | ||
| } | ||
| if (!$hasremote) { | ||
| $errormsg = get_string('connector_sftp:missing_remote', 'tool_dataflows', null, true); |
There was a problem hiding this comment.
This lang string outputs as At least one of source/target must be remote. but there is only one field for the delete step - it might need another string for just Source must be remote.
There was a problem hiding this comment.
The text has been updated for delete and rename
classes/local/step/sftp_trait.php
Outdated
| $mform->addElement('static', 'config_source_desc', '', get_string('connector_sftp:source_desc', 'tool_dataflows'). | ||
| \html_writer::nonempty_tag( | ||
| 'pre', | ||
| get_string('connector_sftp:path_example', 'tool_dataflows'). |
There was a problem hiding this comment.
Needs a different lang string, this one is currently:
Path to the source file. This can be a local file or remote path e.g.
sftp://file/path # Any remote file url;
out.csv # A path relative to the flows temp working dir;
/var/data.json # An absolute path;
file:///my/in.txt # Any valid php stream url;
but in the case of this step, only sftp://file/path is valid
There was a problem hiding this comment.
Also, Source is probably not the most ideal word, File to delete might fit better ?
There was a problem hiding this comment.
This part has been updated
| $errors['config_source'] = $errors['config_source'] ?? $errormsg; | ||
| } | ||
| } else { | ||
| if (!empty($config->source) && !empty($config->target)) { |
There was a problem hiding this comment.
Should this care about $config->target at all? It's not shown as a mform element
There was a problem hiding this comment.
this part is in the behavior "copy" so we do need the target here
|
Another thought - could this also be added as a flow step just for consistency - you should be able to share most of the functionality with the trait |
|
@matthewhilton Thank you for the review. |
gbarat87
force-pushed
the
new-step-move-sftp
branch
from
323bed9 to
e657c3d
Compare
gbarat87
force-pushed
the
new-step-move-sftp
branch
from
e657c3d to
c2c4e61
Compare
This step is to remove a file from the sftp server.
This is needed as there is no possibility to move a file from a folder to another in the sftp server.
The step "sftp copy file" following by this new one can allows one to move a file around.