Feature/image blocking #926

CH3RVB · 2024-04-26T07:24:29Z

This is my first PR to here, so be nice to me /hj

This is my third (i think) crack at making something like this. Everything else was either too clunky or felt... annoying to use? So this is my final solution. LOL.

Adds a modular image blocking system for users. Users can curate their own blocklist by blocking a specific model + ID. Blocked models + IDs have their image blurred out until they become unblocked again by the user. I say model + ID because right now it's an image only thing, but theoretically you could apply this in other ways?

With a small dosage of Moif magic, it automatically applies to all mentions of the blocked image across the site, as long as they hotlink directly to the image URL. Very easy to use across exts, simply plug in the block widget, block an image, and go.

I'm unsure about some aspects, such as the route for the image blocking, as well as how many different world/other entries I should even put this thing on. Maybe the button is also too annoying to look at? Dunno. Either way, it's here :')

itinerare · 2024-04-26T11:26:48Z

app/Helpers/Helpers.php

+
+    if ($block) {
+        return true;
+    } else {


Generally speaking, with returns, you don't need to encase the second in the if/else-- nothing beyond a return is executed to begin with, so you can simply have the first in the if, then the second freestanding.

+1, can even just
return $block ? true : false

actually follow up on this i'd do

function checkImageBlock($image, $user) { return App\Models\ImageBlock::where([ ['image_id', '=', $image->id], ['image_type', '=', get_class($image)], ['user_id', '=', $user->id], ])->exists(); }

itinerare

A good stab at an arguably relatively complex task!

itinerare · 2024-04-26T11:29:04Z

app/Models/ImageBlock.php

+     * @var array
+     */
+    protected $fillable = [
+        'item_id', 'item_type', 'user_id'


Referring to non-specific objects as "items" when "item" is a concept already extant in the codebase is... very confusing.

+1, recommend block_object_id or image_id image_type etc

itinerare · 2024-04-26T11:30:56Z

resources/views/layouts/app.blade.php

+    @if (Auth::check() && count(Auth::user()->blockedImages))
+        <style>
+            @foreach (Auth::user()->blockedImages as $image)
+                [src="{{ $image->item->imageUrl ? $image->item->imageUrl : ($image->item->currencyImageUrl ? $image->item->currencyImageUrl : ($image->item->avatarUrl ? $image->item->avatarUrl : '')) }}"] {


This logic would be better done in an attribute on the image block model, I think. Would be a good use case for a switch statement, potentially.

itinerare · 2024-04-26T11:32:28Z

resources/views/layouts/app.blade.php

@@ -82,6 +82,18 @@
    @endif

    @include('feed::links')
+
+    {{-- Image Blocking --}}
+    @if (Auth::check() && count(Auth::user()->blockedImages))


The count here should just be able to be Auth::user()->blockedImages->count(), since iirc what you get out of a relation is either directly or functionally a collection.

itinerare · 2024-04-26T11:35:07Z

resources/views/widgets/_item_block.blade.php

+     $model = get_class($item);
+ @endphp
+ <input type="hidden" name="item_type" value="{{ $model }}" />
+ {!! Form::submit(checkItemBlock($item, Auth::user()) ? 'Unblock' : 'Block', ['class' => 'btn btn-success btn-sm '.(isset($float) && $float ? 'float-right' : '')]) !!}


Probably this should be clearer about what's being blocked.

itinerare · 2024-04-26T11:39:50Z

resources/views/widgets/_item_block.blade.php

+ @php
+     $model = get_class($item);
+ @endphp
+ <input type="hidden" name="item_type" value="{{ $model }}" />


While the risk is low in a project like this that's already open source, it's generally not the best idea to plunk stuff like model names out in the open; comments already went through this, so have a look at the form for those, etc. for an idea as to what to do about it.

itinerare · 2024-04-26T11:43:14Z

routes/lorekeeper/members.php

@@ -52,6 +52,8 @@
    Route::post('bookmarks/edit/{id}', 'BookmarkController@postCreateEditBookmark');
    Route::get('bookmarks/delete/{id}', 'BookmarkController@getDeleteBookmark');
    Route::post('bookmarks/delete/{id}', 'BookmarkController@postDeleteBookmark');
+
+    Route::post('image-blocks/block/{id}', 'AccountController@postBlockUnblockImage');


If you go a similar route to comments (wrt the above), this'll probably need to be changed accordingly, but the general gist and placement of the route are fine.

itinerare · 2024-04-26T11:44:42Z

app/Services/UserService.php

+
+            //check it's not already blocked, if not, make the block
+
+            $block = ImageBlock::where([


You could probably trim off a little work and express this as $user->blockedImages->where([non-user-id-conditions etc etc])->first() too!

itinerare · 2024-04-26T11:46:37Z

app/Services/UserService.php

+
+        try {
+            $model = $data['item_type'];
+            $item = $model::find($id);


It might be worthwhile to double-check that the object exists just to be safe, since it's technically in the 'accepting semi-arbitrary input from users' zone.

itinerare · 2024-04-26T13:01:22Z

Belatedly, also... it might seem counterintuitive, but it might be an idea to have a page (akin to bookmarks) with the current user's blocked images, as a ready point of reference.

ScuffedNewt

mostly nits, agree with what mercury has said
apologies if comments are short - in work PR review mode

ScuffedNewt · 2024-05-07T22:03:30Z

app/Helpers/Helpers.php

nit: run composer lint to stop these weird style changes

Probably I'm just going to switch the relevant workflow to running on push for all branches again so neither reviewers nor contributors need to think about it, since my clever idea wrt running on PRs only was stymied (fairly reasonably) by repo perms... It was worth a shot |D

👌

It was worth a shot |D

can't work out all the time!

Did that while I was thinking about it-- so it should now be as easy as just merging in latest develop. Hopefully.

ScuffedNewt · 2024-05-07T22:04:30Z

app/Helpers/Helpers.php

+
+    if ($block) {
+        return true;
+    } else {


+1, can even just
return $block ? true : false

ScuffedNewt · 2024-05-07T22:04:59Z

app/Http/Controllers/Users/AccountController.php

again with composer lint

ScuffedNewt · 2024-05-07T22:05:32Z

app/Http/Controllers/Users/AccountController.php

+        ]);
+
+        if ($service->blockUnblockImage($data, $id)) {
+


should this be blank? if yes, change it to !$service... and remove else block

ScuffedNewt · 2024-05-07T22:06:09Z

app/Models/ImageBlock.php

+     * @var array
+     */
+    protected $fillable = [
+        'item_id', 'item_type', 'user_id'


+1, recommend block_object_id or image_id image_type etc

ScuffedNewt · 2024-05-07T22:07:16Z

app/Services/UserService.php

ScuffedNewt · 2024-05-07T22:08:05Z

database/migrations/2024_04_25_052128_add_image_blocking.php

+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+return new class extends Migration


this should be class FileName extends Migration to stop errors on older installs

Does this cause issues? My understanding was that upstream (wrt Laravel) this change was following PHP supporting it, so it doesn't make much sense to me that (assuming PHP is upgraded, which needs to happen regardless) this should be a problem.

I've found on SOME sites it can be an issue, & quiet often on local (people on windows / older xampp)
perhaps in this case it should be fine since this is on v3 which should implicitly mean that the php version is up to par, so take this comment with a grain of salt

I'm inclined to let this be since yeah, v3 explicitly requires PHP be reasonably up-to-date (whereas on current release it's supported but not technically forced, at least by the Laravel version)...

ScuffedNewt · 2024-05-07T22:08:23Z

resources/views/user/_profile_content.blade.php

@@ -6,6 +6,9 @@
    <div class="col-md-2 text-center">
        <!-- User Icon -->
        <img src="{{ $user->avatarUrl }}" class="img-fluid rounded-circle" style="max-height: 125px;" alt="{{ $user->name }}'s Avatar">
+        @if(Auth::check())
+                @include('widgets._item_block', ['item' => $user])
+            @endif


nit: indents

double nit: item should not be the variable name

ScuffedNewt · 2024-05-07T22:09:07Z

resources/views/widgets/_item_block.blade.php

@@ -0,0 +1,7 @@
+ {!! Form::open(['url' => 'account/image-blocks/block/' . $item->id]) !!}
+ @php
+     $model = get_class($item);


further on prior nit: $item is not the best variable name

ScuffedNewt · 2024-05-07T22:09:41Z

resources/views/world/_currency_entry.blade.php

@@ -8,6 +8,9 @@
        <h3>{{ $currency->name }} @if ($currency->abbreviation)
                ({{ $currency->abbreviation }})
            @endif
+            @if(Auth::check())
+                @include('widgets._item_block', ['item' => $currency, 'float' => true])


float seems to be true in most places, consider it being default to true and explicitly pass it as false, but that's mostly preference

ScuffedNewt · 2024-05-07T22:29:59Z

Also unrelated, I find this a little bit strange to include in the default LK install, considering things like toyhouse etc etc don't employ this on general images, and if a specific lorekeeper has such images on items etc that may need to be blocked its best installed individually, imo
@itinerare thoughts?

itinerare · 2024-05-08T00:07:52Z

On some level, I agree... but users may need to block seemingly innocuous stuff in certain cases, so it's not necessarily sufficient to install it on a case-by-case basis/only where more common points of friction occur.

CH3RVB · 2024-05-08T02:28:26Z

Also unrelated, I find this a little bit strange to include in the default LK install, considering things like toyhouse etc etc don't employ this on general images, and if a specific lorekeeper has such images on items etc that may need to be blocked its best installed individually, imo @itinerare thoughts?

On some level, I agree... but users may need to block seemingly innocuous stuff in certain cases, so it's not necessarily sufficient to install it on a case-by-case basis/only where more common points of friction occur.

Is this a good time to add a toggle or leave as-is? Could fulfill both arguments technically, then?

itinerare · 2024-05-08T03:05:24Z

To some degree, I feel like if it's worth including in core at all then there's no sense in undercutting it by putting it behind a toggle...

I think the other part of it is that TH has a different use case than we do, so it's not necessarily practical to make a direct comparison there; TH doesn't really have images that persistently exist around the site in the same way (items, etc.) to be an issue in the first place. Generally speaking, you have more leeway to simply not look at a character or image on there that causes issues, and iirc can block characters-- so it does have some manner of comparable function, just that the context is different.
The more direct analogue would probably be pet sites-- and imo the fact that those generally don't have this kind of option isn't in their favor.

It's one of those things where it's the safer and more considerate thing to be proactive about it and spare folks unnecessary stress, even if it feels like "overkill" in a lot of cases/to include in core.

ScuffedNewt · 2024-05-08T07:51:51Z

The more direct analogue would probably be pet sites

definitely, hit the nail on the head! brain wasn't braining yday

It's one of those things where it's the safer and more considerate thing to be proactive about it and spare folks unnecessary stress, even if it feels like "overkill" in a lot of cases/to include in core.

firmly in the camp of <this is probably not a feature conducive? to core> since most items etc should not be causing these issues ideally, especially when compared to Lorekeepers mainstream counterparts
That being said, I think this would be better placed as having Characters as default -- since when #876 is inevitably cleaned and finished, users may still find that they want to block a certain character even if does is not necessarily fall under a "warning needed" category.
In fact, I think that would be a much more requested feature than items

TLDR; IMO, if this feature is to be added...

Change the primary focus of the default configuration to be for characters
A toggle, if not (since other use cases would need to be manually added by a developer)

ScuffedNewt · 2024-05-08T08:01:20Z

app/Models/ImageBlock.php

+     *
+     * @var string
+     */
+    protected $table = 'image_blocks';


following up on my prior comment about user space too, ideally the table should be user_image_blocks in similar fashion

Likewise, agree on this

itinerare · 2024-05-08T08:17:52Z

definitely, hit the nail on the head! brain wasn't braining yday

Haha, all good, been there.

Anyhow, yeah, I'm inclined to agree that characters are the bigger/more probable use case, so I think it's sensible enough to aim this more firmly at that use case-- with support for other things essentially as a side benefit (albeit one that I do still find in favor of, personally). I'd agree that most items, etc. shouldn't cause issues/it's generally good to avoid making ones that are liable to, but I'd rather provide users the ability to address these things for themselves.

CH3RVB added 3 commits April 25, 2024 04:23

no message

f85f33b

no message

096456c

fixes and automatic CSS

52f8523

itinerare reviewed Apr 26, 2024

View reviewed changes

itinerare requested changes Apr 26, 2024

View reviewed changes

itinerare added enhancement New feature or request needs review Pull requests that are pending community review labels Apr 26, 2024

ScuffedNewt suggested changes May 7, 2024

View reviewed changes

ScuffedNewt reviewed May 8, 2024

View reviewed changes


		//check it's not already blocked, if not, make the block

		$block = ImageBlock::where([

Feature/image blocking #926

Are you sure you want to change the base?

Feature/image blocking #926

Conversation

CH3RVB commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScuffedNewt May 8, 2024 • edited Loading

Choose a reason for hiding this comment

itinerare 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itinerare commented Apr 26, 2024

ScuffedNewt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itinerare May 8, 2024 • edited Loading

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

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

itinerare May 8, 2024 • edited Loading

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

ScuffedNewt commented May 7, 2024

itinerare commented May 8, 2024

CH3RVB commented May 8, 2024

itinerare commented May 8, 2024

ScuffedNewt commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itinerare commented May 8, 2024

ScuffedNewt May 8, 2024 •

edited

Loading

itinerare May 8, 2024 •

edited

Loading

itinerare May 8, 2024 •

edited

Loading