Optionally include tags in Webhook #72

MikeLundahl · 2024-09-13T13:59:00Z

Fixes #0000
Discord & Slack

Changes proposed in this pull request:

Added "Include tags" in the edit webhook modal (Frontend)

Reviewers should focus on:

Screenshot

Settings:

Discord:

Slack:

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).

Required changes:

Related Flarum core extension PR's: (Omit this section if irrelevant)

MikeLundahl · 2024-09-13T16:35:25Z

Fixed some of the formatting things that CI was complaining about.

DavideIadeluca · 2024-09-16T07:36:12Z

Thanks @MikeLundahl for sorting this out. I will get that reviewed today or tomorrow morning.

luceos

I hope it's okay with @DavideIadeluca to leave out Teams, it could be contributed later.

I've taken the time to review your PR and there's one thing I'd prefer seeing changed. The note about formatting you can probably forget about, it occurred only once.

Good job on this one!

luceos · 2024-09-16T07:31:51Z

js/src/admin/components/WebhookEditModal.js

@@ -148,7 +152,8 @@ export default class WebhookEditModal extends Modal {
                      <div>
                        <h3>{this.translate(group)}</h3>
                        {events.map((event) => (
-                          <Switch state={this.webhook.events().includes(event.full)} onchange={this.onchange.bind(this, event.full)}>
+                          <Switch state={this.webhook.events().includes(event.full)}


Formatting changes should be left out. There are a few reasons for that, but the most important one is that it makes the review process so much harder. You really want to just submit the changes that have an actual impact. Especially in open source where you want to make the impact of reviewal as non-existent as possible.

Understand that you could have submitted these changes unknowingly, most IDE's have settings that enable auto formatting before commit. It's best to disable this feature and use the keyboard shortcut to make this a manual action. If per-project settings are possible, then doing that would be an alternative.

I can definitely second that, but as this is such a minor case I wouldn't bother much retroactively now

luceos · 2024-09-16T07:36:35Z

src/Models/Webhook.php

+        $tagsApplied = Tag::whereIn('id', $this->tag_id)->get();
+
+        return $tagsApplied->map(function ($tag) {
+            return $tag->name;
+        })->toArray();


With eloquent it's always a better approach not to retrieve all entries at once. Especially on databases with millions of records this is going to be an issue.

For these use cases you can use a chunking method, each for instance is once of those, but perhaps map() is also a method that chunks: like chunkMap: https://github.com/laravel/framework/blob/05d920410e58f04b13dcaa83937b9d4574fc9b3f/src/Illuminate/Database/Concerns/BuildsQueries.php#L81

Thanks! What about the chunk() method? Did some reading, I could do something like this:

$tagsApplied = []; Tag::whereIn('id', $this->tag_id)->chunk(100, function ($tags) use (&$tagsApplied) { $tagsApplied = array_merge($tagsApplied, $tags->map(function ($tag) { return $tag->name; })->toArray()); }); return $tagsApplied;

If you're only after the name. I would use a select("name") and get(). You won't need to loop or chunk then.

While generally I would agree that chunking is the way to go, I'm not sure if it is of much use here.

Realistically, every community has less than 100 tags, so performance wise it wouldn't really make a difference. I feel like at the point where a community has more than 100 tags, other parts of the ecosystem would be already causing more performance issues.

So in favor of code readability, I would go with the initially suggested method.

The initial one is still unnecessary:

Suggested change

$tagsApplied = Tag::whereIn('id', $this->tag_id)->get();

return $tagsApplied->map(function ($tag) {

return $tag->name;

})->toArray();

return Tag::select('name')->whereIn('id', $this->tag_id)->pluck('name')->toArray();

Is exactly the same thing.

I tried the simplified version with select and pluck... Works on my end so I pushed the tweak. Have a look.

Looks good to me

MikeLundahl · 2024-09-16T14:58:38Z

Thanks @MikeLundahl for sorting this out. I will get that reviewed today or tomorrow morning.

My pleasure! :)

DavideIadeluca

Working as expected – Looks good to me. @luceos please have a look at the conversation about using chunking and let me know if you still would change it up. From my side this PR is good for a merge as it is right now.

DavideIadeluca · 2024-09-17T05:54:09Z

src/Models/Webhook.php

+        $tagsApplied = Tag::whereIn('id', $this->tag_id)->get();
+
+        return $tagsApplied->map(function ($tag) {
+            return $tag->name;
+        })->toArray();


While generally I would agree that chunking is the way to go, I'm not sure if it is of much use here.

Realistically, every community has less than 100 tags, so performance wise it wouldn't really make a difference. I feel like at the point where a community has more than 100 tags, other parts of the ecosystem would be already causing more performance issues.

So in favor of code readability, I would go with the initially suggested method.

DavideIadeluca · 2024-09-17T05:57:55Z

js/src/admin/components/WebhookEditModal.js

@@ -148,7 +152,8 @@ export default class WebhookEditModal extends Modal {
                      <div>
                        <h3>{this.translate(group)}</h3>
                        {events.map((event) => (
-                          <Switch state={this.webhook.events().includes(event.full)} onchange={this.onchange.bind(this, event.full)}>
+                          <Switch state={this.webhook.events().includes(event.full)}


I can definitely second that, but as this is such a minor case I wouldn't bother much retroactively now

js/src/admin/components/WebhookEditModal.js

MikeLundahl added 8 commits September 12, 2024 17:37

New migration to add include_tags column

022b1c7

Added include tags on frontend

fe5f4cb

backend update to handle include tags setting

fafdc1c

Tags working for Discord

555c723

Tags working for Slack

26159ba

removed todo

7b12f68

hide include tags if it is microsoft teams

61f6f4c

Small fix

bc097aa

MikeLundahl mentioned this pull request Sep 13, 2024

Include Tags #71

Closed

MikeLundahl added 3 commits September 13, 2024 18:25

removed double spaces

f7f17d1

added missing comma

4b1f625

Some formatting tweaks

5f97d01

luceos suggested changes Sep 16, 2024

View reviewed changes

DavideIadeluca approved these changes Sep 17, 2024

View reviewed changes

simplified appliedTags

4629cb0

DavideIadeluca changed the title ~~Feature/include tags~~ Optionally include tags in Webhook Sep 17, 2024

DavideIadeluca merged commit 9cd06a7 into FriendsOfFlarum:master Sep 17, 2024
8 checks passed

MikeLundahl deleted the feature/include-tags branch September 17, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally include tags in Webhook #72

Optionally include tags in Webhook #72

MikeLundahl commented Sep 13, 2024

MikeLundahl commented Sep 13, 2024

DavideIadeluca commented Sep 16, 2024

luceos left a comment

luceos Sep 16, 2024

DavideIadeluca Sep 17, 2024

luceos Sep 16, 2024

MikeLundahl Sep 16, 2024

luceos Sep 16, 2024

DavideIadeluca Sep 17, 2024

luceos Sep 17, 2024 •

edited

Loading

MikeLundahl Sep 17, 2024

DavideIadeluca Sep 17, 2024

MikeLundahl commented Sep 16, 2024

DavideIadeluca left a comment

DavideIadeluca Sep 17, 2024

DavideIadeluca Sep 17, 2024

Optionally include tags in Webhook #72

Optionally include tags in Webhook #72

Conversation

MikeLundahl commented Sep 13, 2024

MikeLundahl commented Sep 13, 2024

DavideIadeluca commented Sep 16, 2024

luceos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luceos Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeLundahl commented Sep 16, 2024

DavideIadeluca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luceos Sep 17, 2024 •

edited

Loading