<p></p>
<p><b>@tordans</b> commented on this pull request.</p>
<p dir="auto">This makes the code a lot better <g-emoji class="g-emoji" alias="+1" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f44d.png">👍</g-emoji> .</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3419#discussion_r783643712">app/models/user.rb</a>:</p>
<pre style='color:#555'>> + # Used in test suite, not something that we would normally need to do.
+ event :deactivate do
+ transitions :from => :active, :to => :pending
+ end
</pre>
<p dir="auto">You could make it more explicit that this is just for specs by guarding it or wrapping it in an <code>if Rails.env.test?</code> block (not sure about the exact syntax, just both is just to illustrate).</p>
⬇️ Suggested change
<pre style="color: #555">- # Used in test suite, not something that we would normally need to do.
- event :deactivate do
- transitions :from => :active, :to => :pending
- end
+ # Used in test suite, not something that we would normally need to do.
+ event :deactivate do
+ transitions :from => :active, :to => :pending, : guard => Proc.new { |_| Rails.env.test? }
+ end
</pre>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3419#discussion_r783696849">app/models/user.rb</a>:</p>
<pre style='color:#555'>> + # Mark the account as deleted but keep all data intact
+ event :hide do
+ transitions :from => [:pending, :active, :confirmed, :suspended], :to => :deleted
+ end
+
+ event :unhide do
+ transitions :from => [:deleted], :to => :active
+ end
</pre>
<p dir="auto">This signals to me that hiding is not really a state but a flag on the user. A bit like act_as_paranoid (<a href="https://github.com/rubysherpas/paranoia">and similar</a>) did back in the day.</p>
<p dir="auto">Using a flag would clarify the <code>:unhide</code> event. Right now, a <code>pending</code> user that gets deleted would become <code>active</code> without ever running through the activation steps.</p>
<p dir="auto">Another way to solve this would be to transition to <code>pending</code> within <code>unhide</code> and ask the user to re-take the activation steps.</p>
<hr>
<p dir="auto">However, I wonder what the use case for hiding an account would be. I had assumed that <code>remove_personal_data</code> was applied to all deleted users. Which IMO is desirable since we don't want personal info lying around on accounts that are not used and not visible, right? Could we maybe remove the <code>hide</code> usecase altogether?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3419#discussion_r783703973">app/models/user.rb</a>:</p>
<pre style='color:#555'>> @@ -158,6 +159,51 @@ def self.authenticate(options)
user
end
+ aasm :column => :status, :no_direct_assignment => true do
</pre>
<p dir="auto">Since the user model file is already very long, <a href="https://github.com/openstreetmap/openstreetmap-website/blob/1a11c4dc191d93b18fcf5aa917448c8cd6d2556b/app/models/user.rb#L162-L205">this aasm section</a> would be a good candidate to extract into a concern IMO. However, skimming over <a href="https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/" rel="nofollow">https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/</a> again suggests that a Concern is also smelly, so maybe there is a better way to separate the large file(?).</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3419#pullrequestreview-851267221">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLOMZQP72KIW6I5DU2LUVZ7W7ANCNFSM5LZUSQPA">unsubscribe</a>.<br />Triage notifications on the go with GitHub Mobile for <a href="https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675">iOS</a> or <a href="https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub">Android</a>.
<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLLATT5W2J25P2QZD7LUVZ7W7A5CNFSM5LZUSQPKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOGK6U5FI.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/3419/review/851267221</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/3419#pullrequestreview-851267221",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/3419#pullrequestreview-851267221",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>