[openstreetmap/openstreetmap-website] Allow users to delete their own diary entries (PR #4540)

[Andy Allan]

@gravitystorm requested changes on this pull request.

Happy in principle, just a few minor things to consider here.

> @@ -554,8 +554,8 @@ en:
         other: "%{count} comments"
       no_comments: No comments
       edit_link: Edit this entry
-      hide_link: Hide this entry
-      unhide_link: Unhide this entry
+      delete_link: Delete this entry
+      restore_link: Restore this entry

Is it worth sticking with the original terminology? I think this is the first case where normal users will see a "hide" button (they already see plenty of delete buttons, e.g. traces, messages), but in general the hide/unhide pair is used for reversible actions and delete for actions that can't be undone (even if the delete is really only a soft delete).

I don't want moderators to start hesitating that this might not be the same as the rest of the soft deletes.

> @@ -43,7 +43,7 @@ def initialize(user)
         can [:new, :show, :create, :destroy], :oauth2_authorization
         can [:edit, :update, :destroy], :account
         can [:show], :dashboard
-        can [:new, :create, :edit, :update, :comment, :subscribe, :unsubscribe], DiaryEntry
+        can [:new, :create, :edit, :update, :comment, :subscribe, :unsubscribe, :hide, :unhide], DiaryEntry

I agree with both of you - there's features of CanCanCan that we can use to simplify this, but it's also something that we can tackle separately. Anton's example is correct, we can build permissions within the ability file like

```
if user
  can [:hide], DiaryEntry, :user => user # can only hide their own entries
 ....
if user.moderator?
  can [:hide], DiaryEntry # can hide all entries
```

We can then go back to using the `can :hide, diary_entry` instead of replacing that with inline permission checks like `if current_user && (current_user == diary_entry.user || current_user.moderator?)` which is logic that should be in the abilities system.

I'm happy to have this as a followup task though, particularly since merging this first will make the followup easier due to the additional tests.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4540#pullrequestreview-2071724556
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4540/review/2071724556 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240522/72a4e637/attachment-0001.htm>