add search history settings #1201
Conversation
lukavdplas
force-pushed
the
feature/privacy-settings-1
branch
from
de1d88e to
c6f4236
Compare
There was a problem hiding this comment.
Well done @lukavdplas
Depending on your opinion, this is either an approve or a request changes :)
I would rather not see the merging of User and UserProfile in the frontend, but keep profile as a separate class on a User. I (think) I can follow your reasons, and also find them valid.
| @@ -18,3 +18,20 @@ def has_access(self, corpus_name): | |||
| # check if any corpus added to the user's group(s) match the corpus name | |||
| return any(corpus for group in self.groups.all() | |||
| for corpus in group.corpora.filter(name=corpus_name)) | |||
|
|
|||
|
|
|||
| class UserProfile(models.Model): | |||
There was a problem hiding this comment.
| class UserProfile(models.Model): | |
| class UserProfile(models.Model): | |
| ''' User information that is not relevant to authentication. | |
| E.g. settings, preferences, optional personal information. | |
| ''' |
backend/users/signals.py
Outdated
| if created: | ||
| UserProfile.objects.get_or_create( | ||
| user=instance | ||
| ) |
There was a problem hiding this comment.
This combination makes no sense. If a User is created, there should not be a UserProfile because it's a 1-to-1 relation. Cases where the get part is executed instead are clear bugs and should not be passed silently.
| if created: | |
| UserProfile.objects.get_or_create( | |
| user=instance | |
| ) | |
| if created: | |
| UserProfile.objects.create(user=instance) |
| let resultsPromise: Promise<SearchResults>; | ||
|
|
||
| if (user.enableSearchHistory) { | ||
| resultsPromise = this.searchAndSave(queryModel, user, request); | ||
| } else { | ||
| resultsPromise = request(); | ||
| } |
There was a problem hiding this comment.
I'm not always a fan of ternary operator, but here it saves a lot of lines. Feel free to ignore.
| let resultsPromise: Promise<SearchResults>; | |
| if (user.enableSearchHistory) { | |
| resultsPromise = this.searchAndSave(queryModel, user, request); | |
| } else { | |
| resultsPromise = request(); | |
| } | |
| const resultPromise: Promise<SearchResults> = user.enableSearchHistory ? | |
| this.searchAndSave(queryModel, user, request) : request(); |
frontend/src/app/settings/search-history-settings/search-history-settings.component.html
Show resolved
Hide resolved
| export class SearchHistorySettingsComponent implements OnInit { | ||
|
|
||
| constructor() { } | ||
|
|
||
| ngOnInit(): void { | ||
| } |
There was a problem hiding this comment.
Tiniest hill to die on.
| export class SearchHistorySettingsComponent implements OnInit { | |
| constructor() { } | |
| ngOnInit(): void { | |
| } | |
| export class SearchHistorySettingsComponent { | |
| constructor() { } |
| templateUrl: './settings.component.html', | ||
| styleUrls: ['./settings.component.scss'] | ||
| }) | ||
| export class SettingsComponent implements OnInit { |
Fair enough, I wasn't sure about this myself. I would like these to be handled in a single view, but |
Ah, I remembered the implementation incorrectly, so this reply makes no sense. It's already nested. I can see the merit of having one view for authentication and one for user information, but the current implementation already merges those. I'm not sure that separation should be included in this PR. |
Adds a settings screen where users can configure whether to enable search history, or delete their existing history. Close #1145