Refactorize and test microservice/recordings #2826
Conversation
danigargar
added
+enhancement
danigargar
force-pushed
the
PROVIDER-2014-recordings-microservice-refactor
branch
3 times, most recently
from
4ae6502 to
eb4d3b9
Compare
danigargar
force-pushed
the
PROVIDER-2014-recordings-microservice-refactor
branch
3 times, most recently
from
7973e0c to
ff39243
Compare
danigargar
force-pushed
the
PROVIDER-2014-recordings-microservice-refactor
branch
from
ff39243 to
94c01dc
Compare
danigargar
force-pushed
the
PROVIDER-2014-recordings-microservice-refactor
branch
3 times, most recently
from
3a085c6 to
df2fbbd
Compare
danigargar
force-pushed
the
PROVIDER-2014-recordings-microservice-refactor
branch
from
df2fbbd to
e6989bc
Compare
|
|
||
| public function getCallid(): ?string | ||
| { | ||
| return ($this->callid); |
| $this->rawRecordingsDir, | ||
| $filename | ||
| ); | ||
|
|
There was a problem hiding this comment.
I would add a check to only consider actual files and skip directories, links, and so on. The rawRecordingsDir may contain other files that are not actually Recordings.
Also, considering only files that ends with -mix.wav could be a good idea, because other files in the directory that doesn't match that pattern should be ingored, and not considered as errors.
There was a problem hiding this comment.
Maybe this is also a good point to validate file format to extract callid from the filename
There was a problem hiding this comment.
This service is here only keep file system functions separated from the logic. It's fully mocked in testing.
| $this->rawRecordingInfoMockFactory( | ||
| '/recordings/wrong-file-name.ogg', | ||
| Encoder::RECORDING_SIZE_MIN + 1, | ||
| Encoder::RECORDING_AGE_MIN + 1 | ||
| ), |
There was a problem hiding this comment.
This file should be ignored by the recording microservice
| if (preg_match("/\w+-\w+-(.*)-\w+-mix.wav/", $this->filename, $matches)) { | ||
| $this->callid = urldecode($matches[1]); | ||
| $this->hashid = substr(md5($this->callid), 0, 8); | ||
| } |
There was a problem hiding this comment.
I will validate this before creating a RawRecordingInfo instance. If this condition doesn't match, it's not a recording and doesn't deserve a RawRecordingInfo. Also $this->callid and $this->hashid shouldn't be nullable. If the file doesn't have those values, it can not be considered a recording, so callid could be part of the constructor imo.
| $billableCalls = $this | ||
| ->billableCallRepository | ||
| ->findByCallid( | ||
| $filename->getCallid() ?? '' |
There was a problem hiding this comment.
As mentioned before, this should not happen. Callid is a must in the recording and should not be nullable.
Kaian
force-pushed
the
bleeding
branch
2 times, most recently
from
df24ab3 to
842b736
Compare
Type Of Change
Checklist:
Description
Additional information