Accept terms of use #322

gahermar · 2022-03-20T18:29:21Z

Accept term of use after login for all users
After change term of use set status accepted on all users to false.

app/assets/stylesheets/application.css.scss

app/controllers/application_controller.rb

app/controllers/sites_controller.rb

jiri1337 · 2022-03-21T10:38:57Z

app/controllers/sites_controller.rb

+    if(@site.terms_of_use_content_changed?)
+      id = current_user.id
+      users = User.where("id != ?", id)
+      users.update_all(terms_of_use_accepted: false)


This is one possible way of course. What would you think about a solution that uses terms_accepted_at (DateTime) on a user model - this way you would simply compare updated_at on Site with the terms_accepted_at on a model to check whether the user has accepted the updated terms.

Yeah this solution is nice to. I following instructions in task (#317)
And it depend ... if we need to save date of accepted it will be better your solution. but if we wouldn't need it, i think solution with boolean is better .... for example when we need check in DB who accept and who not it will be more readeble when we have this data. Of course we can have both or it is just simple sql query to show it but if we no need date of accept i think it is useles.

Since you introduced terms_of_use_accepted_date the boolean field terms_of_use_accepted is a duplicate. I'd would stay with terms_of_use_accepted_date on the user and simply add a virtual field on user (pseudo code):

def terms_of_use_accepted user.terms_of_use_accepted_date < site.terms_of_use_accepted_date end

Then you don't have to update all database rows when the terms change.

yes, good tip. I changed it..

jiri1337 · 2022-03-21T10:42:41Z

app/controllers/users_controller.rb

+class UsersController < ApplicationController
+
+  def edit_terms
+    if(current_user != nil)


This project is using cancancan and devise - I think there are better ways to ensure the user is not nil or that the user has to be authorized to perform this action.
This of course will work but it's not the cleanest solution.

Also, you could use .nil? and you don't need to find the user when you have current_user available in this context - it already has found the user for you.

I must say i need to look more on cancancan ....

i was repair nil and remove finding user.

jiri1337 · 2022-03-21T10:45:36Z

app/controllers/users_controller.rb

+    if current_user.is_a?(Mentor) && current_user.kids.empty?
+      # if a menotr has no kids yet assigned, go to available kids
+      redirect_to available_kids_path
+    elsif current_user.is_a?(Teacher) && current_user.mentor_matchings.pluck(:state).include?('pending')


This is a way but the performance could be better-using database query -> using where or exists?

I came out of the solution which is in application_controller.
And i don't understand what you mean with DB query. I thought DB queries are more difficult..

Ah I meant something in this manner (using Activerecord) current_user.mentor_matchings.exists?(state: 'pending')

Ah this. ok. I changed it. And now question - we have same things in application controller after login. Change it too? I just ask because it is not my code but on the other side it is improve.

app/views/layouts/application.html.haml

jiri1337 · 2022-03-21T10:56:23Z

spec/requests/site_configuration_spec.rb

@@ -25,4 +25,32 @@
    expect(page).to have_css('h1', text: 'last1, first1')
    expect(page).to have_css('h2', text: 'Gesprächsdokumentationen')
  end
+


It would be cool to check if

After updating the terms, the terms_of_use_accepted gets reset for other users that have previously accepted the terms

Checking whether the user's terms_of_use_accepted change from false to true on the database level

I think the first check what you want is test on line 29 - i create admin and teacher (both with akcepted = true) then i login with admin - did some change in terms of use and save. Logout and login with teacher and check if there is showed page with term of use.

Second check - i tried to do new test in same file on line 57 - similar function but at the end i was reload teacher and check terms_of_use_accepted. Did you mean that?

config/locales/future_kids.de.yml

repairs

panterch · 2022-03-23T12:24:22Z

Dear @MartinGaher

Thanks for your work - very much appreciated!

I've checked out the branch and did some acceptance testing. Here are my findings:

The following acceptance criteria seems not to be fulfilled:

Werden die Nutzungsbedingungen erneut geändert, müssen diese erneut angezeigt und akzeptiert werden

In the implementation notes there were some hints how to achive this. Feel free to find another way of implementing it, that fulfills the same criterias:

Auf dem site model soll aufgezeichnet, wann die Nutzungsbedingungen das letzte mal geändert wurden. Dies kann z.B. über den "Dirty" state des Feldes gemacht werden: https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Dirty.html

Auf dem User Model muss ebenfalls aufgezeichnet werden, wann Benutzer*innen das letzte Mal die Nutzungsbedingungen akzeptiert haben.

I tested by logging in as admin, accepting the tos, then changing the tos, logging out and login in again. It should have displayed me the tos again but did not.

gahermar · 2022-03-23T13:44:41Z

Hello @panterch ,
sorry for that. I usually do not enforce accept for user which making changes - it was for all other users. I repair it and now after save app wants to accept too for admin which make changes. I also added datetime to user for check last accepted date and also datetime to site for check last update date.

app/views/layouts/application.html.haml

jiri1337 · 2022-03-23T18:34:30Z

app/controllers/sites_controller.rb

+      @site.terms_of_use_changed_date = DateTime.now
+    end
+
+    if @site.save


I see one little issue here - What if theoretically speaking saving failed. In this case, we would still set terms_of_use_accepted to false for all users.
This is not a big issue but it could maybe be addressed as well.

There's a convention to end date fields with _at. Maybe you rename this to site.terms_of_use_changed_at to make the code more readable.

Ok. when i changed logic to dates it is unnecessarily now.

i changed column names to _at

panterch · 2022-03-23T19:28:44Z

db/migrate/20220323125356_add_terms_of_use_accepted_date_to_users.rb

@@ -0,0 +1,5 @@
+class AddTermsOfUseAcceptedDateToUsers < ActiveRecord::Migration[6.1]
+  def change
+    add_column :users, :terms_of_use_accepted_date, :datetime


There's a convention to end date fields with _at. Maybe you rename this to terms_of_use_accepted_at to make the code more readable.

i changed column names to _at

Change names tos columns in Db, compute accepted from dates + some small changes

gahermar · 2022-03-24T18:01:05Z

Hi,
i have question about default values. Now i changed columns in DB to have default values to datetime.now. It is ok or it is better to let it nil and do some check in code? It depends on how is user created. If there is some acceptation during creation or not or if some other user create user account?

MartinGaher added 4 commits March 17, 2022 20:33

Test task - 317 - Accept Terms of use for users

5b39178

repair tests

da355a4

tests

b50c66a

add headline + remove decline button

0c852e1