#116 - integration with slack #129

2022-04-25T13:34:53+02:00

Baakoma commented

2022-04-25 13:34:53 +02:00

(Migrated from github.com)

It should close #116
It should close #117
It should close #119
It should close #121

![image](https://user-images.githubusercontent.com/16743001/165081105-79bc4b63-1770-49e1-977c-94a8cf7bf49a.png) It should close #116 It should close #117 It should close #119 It should close #121

krzysztofrewak (Migrated from github.com) requested changes 2022-04-26 10:19:25 +02:00

app/Domain/DailySummaryRetriever.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:07:42 +02:00

            ->whereTypes(VacationType::all()->filter(fn(VacationType $type): bool => $this->configRetriever->isVacation($type)))

```suggestion ->whereTypes(VacationType::all()->filter(fn(VacationType $type): bool => $this->configRetriever->isVacation($type))) ```

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:07:53 +02:00

            ->whereTypes(VacationType::all()->filter(fn(VacationType $type): bool => !$this->configRetriever->isVacation($type)))

```suggestion ->whereTypes(VacationType::all()->filter(fn(VacationType $type): bool => !$this->configRetriever->isVacation($type))) ```

app/Domain/Notifications/KeyHasBeenGivenNotification.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:08:38 +02:00

Maybe these "slack" and other channels could be gathered somewhere and used as constants or enums?

Maybe these `"slack"` and other channels could be gathered somewhere and used as constants or enums?

app/Domain/Slack/Channels/SlackApiChannel.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:09:19 +02:00

Can you type $notifiable?

Can you type `$notifiable`?

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:09:51 +02:00

Hm, I don't if I wouldn't put his to Infrastucture directory.

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:11:01 +02:00

Could you extract this config to separate method? It would make further testing easier, I think.

app/Domain/Slack/Controller.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:11:59 +02:00

Please add return type here.

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:12:17 +02:00

@EwelinaLasowy , what do you think about word Komenda? :>

app/Domain/Slack/Handlers/DailySummary.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:13:22 +02:00

        $absences = $dailySummaryRetriever->getAbsences($now)
            ->map(fn(Vacation $vacation): string => $vacation->user->profile->full_name);

        $remoteDays = $dailySummaryRetriever->getRemoteDays($now)
            ->map(fn(Vacation $vacation): string => $vacation->user->profile->full_name);

        $birthdays = $dailySummaryRetriever->getBirthdays($now)
            ->map(fn(User $user): string => $user->profile->full_name);

```suggestion $absences = $dailySummaryRetriever->getAbsences($now) ->map(fn(Vacation $vacation): string => $vacation->user->profile->full_name); $remoteDays = $dailySummaryRetriever->getRemoteDays($now) ->map(fn(Vacation $vacation): string => $vacation->user->profile->full_name); $birthdays = $dailySummaryRetriever->getBirthdays($now) ->map(fn(User $user): string => $user->profile->full_name); ```

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:14:04 +02:00

What if we would have five more activities? Maybe this could be encapsulated somehow?

app/Domain/Slack/Traits/FindsUserBySlackId.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:16:44 +02:00

Is it required here? https://github.com/laravel/framework/blob/9.x/src/Illuminate/Collections/Collection.php#L387

app/Infrastructure/Console/Commands/SendDailySummaryToSlack.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:18:03 +02:00

These attachments are almost the same as somewhere above. There should be some abstraction for that.

krzysztofrewak (Migrated from github.com) commented

2022-04-26 10:18:38 +02:00

I think I would prefer to use camelCased model attributes. It's not for this PR, but please think about it.

Baakoma (Migrated from github.com) reviewed 2022-04-26 10:58:05 +02:00

app/Domain/Slack/Traits/FindsUserBySlackId.php

Baakoma (Migrated from github.com) commented

2022-04-26 10:58:05 +02:00

Yes.
Return value is expected to be 'null|\Toby\Eloquent\Models\User', '\Illuminate\Database\Eloquent\Builder|\Illuminate\Database\Eloquent\Model|object' returned

Yes. `Return value is expected to be 'null|\Toby\Eloquent\Models\User', '\Illuminate\Database\Eloquent\Builder|\Illuminate\Database\Eloquent\Model|object' returned `

Baakoma (Migrated from github.com) reviewed 2022-04-26 11:00:45 +02:00

app/Domain/Slack/Channels/SlackApiChannel.php

Baakoma (Migrated from github.com) commented

2022-04-26 11:00:45 +02:00

In general, it can be anything that uses trait Notifiable

In general, it can be anything that uses trait `Notifiable`

krzysztofrewak (Migrated from github.com) reviewed 2022-04-26 11:17:22 +02:00

app/Domain/Slack/Channels/SlackApiChannel.php

krzysztofrewak (Migrated from github.com) commented

2022-04-26 11:17:21 +02:00

Oh no, maybe you could add some interface fot that?

EwelinaLasowy (Migrated from github.com) reviewed 2022-04-26 14:23:38 +02:00

app/Infrastructure/Slack/Controller.php

EwelinaLasowy (Migrated from github.com) commented

2022-04-26 14:21:14 +02:00

            ->withText(":x: Polecenie `/{$this->request->command} {$this->request->text}` jest niepoprawne:")

```suggestion ->withText(":x: Polecenie `/{$this->request->command} {$this->request->text}` jest niepoprawne:") ```

tests/Unit/SendDailySummaryToSlackTest.php

EwelinaLasowy (Migrated from github.com) commented

2022-04-26 14:22:36 +02:00

    public function testCommandDoesntSendMessageIfWeekend(): void

```suggestion public function testCommandDoesntSendMessageIfWeekend(): void ```

EwelinaLasowy (Migrated from github.com) commented

2022-04-26 14:23:25 +02:00

    public function testCommandDoesntSendMessageIfHoliday(): void

```suggestion public function testCommandDoesntSendMessageIfHoliday(): void ```

EwelinaLasowy (Migrated from github.com) approved these changes 2022-04-27 09:33:36 +02:00

krzysztofrewak (Migrated from github.com) approved these changes 2022-04-27 09:52:38 +02:00

app/Domain/Notifications/Notifiable.php

krzysztofrewak (Migrated from github.com) commented

2022-04-27 09:14:59 +02:00

👀

:eyes:

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kamilniemczycki/toby#129