[openstreetmap/openstreetmap-website] Account deletion cool-down period (PR #4313)
Andy Allan
notifications at github.com
Thu Nov 16 10:07:58 UTC 2023
@gravitystorm requested changes on this pull request.
Overall I'm happy with the proposed feature and I'd like to get it implemented soon. But there's a number of issues with the current proposal that will need to be fixed first, so I've made a detailed review.
If anything needs clarifying or further discussion, please let me know.
> @@ -33,6 +33,10 @@ def friendly_date_ago(date)
tag.span(time_ago_in_words(date, :scope => :"datetime.distance_in_words_ago"), :title => l(date, :format => :friendly))
end
+ def friendly_date_time_element(date)
I was considering this yesterday, since this helper is more or less a duplicate of the `friendly_date` helper, and I don't think having two very similar helpers is a good idea. But rather than explaining everything and asking for a "prerequistes PR", I've done the work myself in https://github.com/openstreetmap/openstreetmap-website/pull/4349
So you can now rebase and use the `friendly_date` helper, which will get you the `time` element and the machine-readable date that you are using in the tests. Note that the attribute is `datetime` and not `timestamp`, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/time , so your tests will need updating too.
> @@ -7,6 +7,8 @@ class DeletionsController < ApplicationController
authorize_resource :class => false
- def show; end
+ def show
+ @allowed_at = current_user.deletion_allowed_at
I don't think this indirection is necessary - if the `.deletion_allowed_at` method is expensive then we can memoise it in the model, but given it's only called twice and the query will be cached it's probably not worth it. So you can use current_user.deletion_allowed_at directly in the view.
> @@ -407,6 +407,14 @@ def max_changeset_comments_per_hour
end
end
+ def deletion_allowed_at
I'm not super keen on having a method with is both used as a boolean (in reality, testing for nil) and also to return a date. I think the code will be easier to understand if there's a `user.deletion_allowed?` method, and then the `user.deletion_allowed_at` can be a date without the additional `nil` considerations.
> @@ -407,6 +407,14 @@ def max_changeset_comments_per_hour
end
end
+ def deletion_allowed_at
+ last_changeset = changesets.first
+ if last_changeset
+ earliest_possible_time = last_changeset.created_at + Settings.user_account_deletion_delay.hours
I've spent a lot of time reviewing this bit, since the current code creates some unexpected behaviour.
The core purpose of the cooldown period is to provide a gap between the last change to the data, and the account being deleted, to facilitate community review. However, by using the changeset.created_at, this becomes unpredictable, particularly with shorter cooldown durations.
I've tried to explain this with some ASCII art, where `o` is a changeset created_at, `c` is changeset closed_at, `x` is a diff upload and `d` is the account being deleted. Each `.` is roughly an hour but I'm not counting precisely.
If the cooldown period is around 24 hours (i.e. comparable to max changeset duration), then there can be wildly varying gaps between the last change and the account deletion. For example:
```
Changeset | o.xc
User | ........................d
Changeset | o....................xc
User ........................d
```
You can see that the gap between the `x` (data change) and `d` (account deletion) can be very short, much less than expected. As the cooldown period increases, the percentage difference is less, but the cooldown period needs to be more than about 7 days before it's not really relevant. I would like to have the behaviour predictable regardless of the setting. As currently implemented, a cooldown period of 24 hours can lead to accounts closing only ~1 hour after the last change, which would be unexpected.
Note that the code also uses the last opened changeset, which again leads to unpredictability when there are two open changesets, e.g.
```
Changeset A | o....................xc
Changeset B | o.xc
User | ........................d
```
Although B is the last opened changeset, edits can be made on other, older changesets too.
As far as I can see, there are two approaches that we could use to solve this:
1) Keep basing the period based on `created_at`, but add `Changeset.MAX_TIME_OPEN`. This will still lead to an unpredictable gap, but at least it will always be greater than `Settings.user_account_deletion_delay`. For example:
```
Changeset | o.xc
User | [mmmmmmmmmmmmmmmmmmmm]........................d
Changeset | o....................xc
User | [mmmmmmmmmmmmmmmmmmmm]........................d
```
There are still different gaps between the `x` and `d`, however it's probably more helpful than the previous situation (i.e. for a setting of `24` it's 24..48 rather than 1..24, as currently implemented.
However, this then means that if you put the setting to `0`, that doesn't disable it and accounts still have a cooldown period of up to Changeset.MAX_TIME_OPEN hours, even if you don't want that feature any more.
2) Add an index to changesets.closed_at, and use that in the calculation instead. This is my preferred solution. This means that the `Settings.user_account_deletion_delay` configuration value will behave predictably and it will be much easier to reason about what is going on. All changesets have a closed_at time; account closure will be directly related to the last edit, and adding more edits to any changeset will automatically push back the account closure.
> @@ -53,6 +53,8 @@ api_timeout: 300
web_timeout: 30
# Periods (in hours) which are allowed for user blocks
user_block_periods: [0, 1, 3, 6, 12, 24, 48, 96, 168, 336, 731, 4383, 8766, 87660]
+# Account deletion cooldown period (in hours) since last changeset close; 0 to disable, 24 to make sure there aren't any open changesets when the deletion happens
The comment here doesn't match the implementation, as discussed, but we can revisit this after updating the core logic.
> + user = create(:user)
+ create(:changeset, :user => user)
+ session_for(user)
+
+ get account_deletion_path
+ assert_response :success
+ assert_select ".btn", "Delete Account" do
+ assert_select ">@disabled", 0
+ end
+ end
+ end
+
+ def test_past_delay
+ with_user_account_deletion_delay(48) do
+ user = create(:user)
+ (11..15).reverse_each do |n|
There's no need to create 5 changesets when one will do.
If the test is to make sure that the account can be deleted if the changeset is older than X hours, then the tests should cover the scenarios X-1, X, X+1 and assert the behaviour around the boundary condition. That's a better approach than testing 5 changesets that are nowhere near the boundary.
> + create(:changeset, :user => user, :created_at => Time.now.utc - n.days)
+ end
+ session_for(user)
+
+ get account_deletion_path
+ assert_response :success
+ assert_select ".btn", "Delete Account" do
+ assert_select ">@disabled", 0
+ end
+ end
+ end
+
+ def test_delay
+ with_user_account_deletion_delay(48) do
+ user = create(:user)
+ (1..5).reverse_each do |n|
As above, only one changeset is necessary here, and it should be next to (or on) the boundary, i.e. either 47 or 48 hours, depending on the implementation.
> @@ -0,0 +1,65 @@
+require "test_helper"
+
+class DeletionsControllerTest < ActionDispatch::IntegrationTest
In general, if we are testing the user interface, we should do that with system tests. These are more readable and maintainable than doing xpath-style selects on the raw documents.
The controller tests should be kept for testing edge case behaviour, e.g. when sending incorrect information to the controller, that the UI wouldn't normally allow. So the accounts_controller_test part is good, since it's checking that people can't post deletions directly to the controller.
I'm well aware that our current test suite has an awful lot of UI testing in controller tests, but for new features we should use system tests. We also have system tests already for account deletion (and not controller tests) so let's stick with that.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4313#pullrequestreview-1733880974
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/4313/review/1733880974 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20231116/67fc24e1/attachment-0001.htm>
More information about the rails-dev
mailing list