Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accept terms of use #322
base: develop
Are you sure you want to change the base?
Accept terms of use #322
Changes from 4 commits
5b39178
da355a4
b50c66a
0c852e1
de09837
58aeb96
b57bafc
a9c9ecc
8b060fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 compareupdated_at
on Site with theterms_accepted_at
on a model to check whether the user has accepted the updated terms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
Then you don't have to update all database rows when the terms change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good tip. I changed it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one little issue here - What if theoretically speaking saving failed. In this case, we would still set
terms_of_use_accepted
tofalse
for all users.This is not a big issue but it could maybe be addressed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a convention to end date fields with
_at
. Maybe you rename this tosite.terms_of_use_changed_at
to make the code more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. when i changed logic to dates it is unnecessarily now.
i changed column names to _at
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project is using
cancancan
anddevise
- 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 havecurrent_user
available in this context - it already has found the user for you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must say i need to look more on cancancan ....
i was repair nil and remove finding user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a way but the performance could be better-using database query -> using where or exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I meant something in this manner (using Activerecord)
current_user.mentor_matchings.exists?(state: 'pending')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool to check if
terms_of_use_accepted
gets reset for other users that have previously accepted the termsterms_of_use_accepted
change from false to true on the database levelThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?