vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat: save and restore the user language on the server #1181

Merged
konrad merged 4 commits from feature/persist-user-language into main 2021-12-30 20:20:46 +00:00
Conversation 3 Commits 4 Files Changed 2 +67 -39
konrad commented 2021-12-12 14:45:59 +00:00
Owner
Copy Link

This PR persists the current user language on the server, either when the user has no language set yet or when explicitely saving it in the settings. It restores the saved language when the user logs in.

Will allow for things like localized emails in the future.

Depends on a98119f2d6

This PR persists the current user language on the server, either when the user has no language set yet or when explicitely saving it in the settings. It restores the saved language when the user logs in. Will allow for things like localized emails in the future. Depends on https://kolaente.dev/vikunja/api/commit/a98119f2d670a11efab6008129b767f9208f8113
konrad requested review from dpschen 2021-12-12 14:47:22 +00:00
dpschen requested changes 2021-12-12 15:43:02 +00:00
src/store/modules/auth.js Outdated
@ -1,6 +1,8 @@
import {HTTPFactory} from '@/http-common'
import {getCurrentLanguage, saveLanguage, setLanguage} from '../../i18n'
dpschen commented 2021-12-12 15:23:05 +00:00
Member

Use @

Use `@`
konrad marked this conversation as resolved
src/store/modules/auth.js Outdated
@ -224,0 +227,4 @@
if (info.settings.language !== '') {
saveLanguage(info.settings.language)
} else {
await ctx.dispatch('saveCurrentLanguage')
dpschen commented 2021-12-12 15:40:56 +00:00
Member

If info.settings.language !== '' we save the current language from vue-i8n.

Why? Shouldn't that be a user action?

Basically:
=> no language selected
=> automatic language by user language

If I understood this correct this might be a side effect from some unrelated settings change.

If `info.settings.language !== ''` we save the current language from vue-i8n. Why? Shouldn't that be a user action? Basically: => no language selected => automatic language by user language If I understood this correct this might be a side effect from some unrelated settings change.
konrad commented 2021-12-12 19:00:30 +00:00
Author
Owner

Why? Shouldn't that be a user action?

Basically:
=> no language selected
=> automatic language by user language

When the user configured their browser so it automatically changes the website language to the one preferred by the user, they might never visit the language settings since the language is already correct. I still want to save the current browser language in these cases.

If I understood this correct this might be a side effect from some unrelated settings change.

I don't really see a problem here because this happens right after the settings were retrieved from the server.

> Why? Shouldn't that be a user action? > > Basically: > => no language selected > => automatic language by user language When the user configured their browser so it automatically changes the website language to the one preferred by the user, they might never visit the language settings since the language is already correct. I still want to save the current browser language in these cases. > If I understood this correct this might be a side effect from some unrelated settings change. I don't really see a problem here because this happens right after the settings were retrieved from the server.
👍 1
konrad marked this conversation as resolved
src/views/user/settings/General.vue Outdated
@ -209,3 +211,1 @@
await this.userSettingsService.update(this.settings)
this.$store.commit('auth/setUserSettings', {
...this.settings,
await this.$store.dispatch('auth/saveUserSettings', {
dpschen commented 2021-12-12 15:37:28 +00:00
Member

Remove this.userSettingsService + import since it's not used now anymore.

Remove `this.userSettingsService` + import since it's not used now anymore.
konrad commented 2021-12-12 19:05:19 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
frederick commented 2021-12-12 16:03:46 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1181-featurepersist-user-language--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1181-featurepersist-user-language--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen reviewed 2021-12-13 13:04:38 +00:00
src/views/user/settings/General.vue Outdated
@ -182,2 +183,4 @@
return this.$store.getters['lists/getListById'](this.settings.defaultListId)
},
loading() {
return this.$store.state[LOADING] && this.$store.state[LOADING_MODULE] === 'general-settings'
dpschen commented 2021-12-13 13:04:38 +00:00
Member

LOADING and LOADING_MODULE are both not mutations but states.
=> should not use import from mutation-types.

Btw: I never got how this worked when multiple modules load at the same time.
Don't they overwrite the loadingModule state (loading state wouldn't matter so much I guess)?

`LOADING` and `LOADING_MODULE` are both not mutations but states. => should not use import from mutation-types. Btw: I never got how this worked when multiple modules load at the same time. Don't they overwrite the `loadingModule` state (`loading` state wouldn't matter so much I guess)?
konrad commented 2021-12-14 19:50:49 +00:00
Author
Owner

LOADING and LOADING_MODULE are both not mutations but states.
=> should not use import from mutation-types.

I think there's a few more places where this is wrong as well.

Don't they overwrite the loadingModule state (loading state wouldn't matter so much I guess)?

They do. That's one of the issues with it but I primarily wanted a way to check whether a certain module is loading right now without exposing the loading property of the service. I think the proper way to do this would be to add all kinds of loading properties to the modules?

> LOADING and LOADING_MODULE are both not mutations but states. => should not use import from mutation-types. I think there's a few more places where this is wrong as well. > Don't they overwrite the loadingModule state (loading state wouldn't matter so much I guess)? They do. That's one of the issues with it but I primarily wanted a way to check whether a certain module is loading right now without exposing the `loading` property of the service. I think the proper way to do this would be to add all kinds of loading properties to the modules?
konrad commented 2021-12-14 20:13:25 +00:00
Author
Owner

How would you reccommend solving this? Adding new consts to a new file state-type similar to the one holding the mutation types?

How would you reccommend solving this? Adding new consts to a new file `state-type` similar to the one holding the mutation types?
dpschen commented 2021-12-15 21:14:16 +00:00
Member

I think the proper way to do this would be to add all kinds of loading properties to the modules?

I have something in mind for this. But I guess it makes sense to talk about this when we get there. Will probably when we start using pinia.

Adding new consts to a new file state-type similar to the one holding the mutation types?

Then you could not use the same state-type at the same time because there could be conflicts. Probably you shouldn't either way because merging them would benefit speed, but you never know when this case might happen.

> I think the proper way to do this would be to add all kinds of loading properties to the modules? I have something in mind for this. But I guess it makes sense to talk about this when we get there. Will probably when we start using pinia. > Adding new consts to a new file state-type similar to the one holding the mutation types? Then you could not use the same `state-type` at the same time because there could be conflicts. Probably you shouldn't either way because merging them would benefit speed, but you never know when this case might happen.
dpschen commented 2021-12-15 21:16:18 +00:00
Member

For now I would still remove the impor tof the mutation. For pinia this is not necessary because of the better typing.
You can also add better types to vuex but I guess this is now not worth the effort anymore. So going forward we will need to this kind of stuff either way =)

For now I would still remove the impor tof the mutation. For pinia this is not necessary because of the better typing. You can also add better types to vuex but I guess this is now not worth the effort anymore. So going forward we will need to this kind of stuff either way =)
dpschen marked this conversation as resolved
dpschen reviewed 2021-12-13 13:06:31 +00:00
src/views/user/settings/General.vue Outdated
@ -206,9 +210,11 @@ export default {
setQuickAddMagicMode(this.quickAddMagicMode)
this.settings.defaultListId = this.defaultList ? this.defaultList.id : 0
dpschen commented 2021-12-13 13:06:31 +00:00
Member

The local settings should be changed only after the user settings are saved, so that in case something goes wrong we don't have a wrong local representation of the state.

The local settings should be changed only after the user settings are saved, so that in case something goes wrong we don't have a wrong local representation of the state.
konrad commented 2021-12-14 20:10:40 +00:00
Author
Owner

But wouldn't that require a new data variable? That feels kind of clunky because then I need to synchronize it again when loading data initially from the server etc. Maybe the error message is enough to tell the user "hey something went wrond while saving and what you see in the interface isn't nessecarily what is saved"?

But wouldn't that require a new data variable? That feels kind of clunky because then I need to synchronize it again when loading data initially from the server etc. Maybe the error message is enough to tell the user "hey something went wrond while saving and what you see in the interface isn't nessecarily what is saved"?
dpschen commented 2021-12-15 21:21:00 +00:00
Member

This was kind of solved by the default list branch.
Reworking how the settings are saved would be a different topic =)

This was kind of solved by the default list branch. Reworking how the settings are saved would be a different topic =)
dpschen marked this conversation as resolved
dpschen force-pushed feature/persist-user-language from 9d66a9d4c2 to 94098a66a6 2021-12-17 16:47:42 +00:00 Compare
dpschen requested changes 2021-12-17 16:50:49 +00:00
dpschen left a comment
Member

I updated the stuff I was mentioning and force-pushed these changes.

But I found one more issue.
Hope the force-push was fine

I updated the stuff I was mentioning and force-pushed these changes. But I found one more issue. Hope the force-push was fine
src/views/user/settings/General.vue
@ -164,7 +162,6 @@ export default {
language: getCurrentLanguage(),
dpschen commented 2021-12-17 16:49:20 +00:00
Member

This seems wrong:
Now that we have a language field in the settings the current language should be loaded from the settings and only loaded from the browsers if a user language was not yet defined.

This seems wrong: Now that we have a language field in the settings the current language should be loaded from the settings and only loaded from the browsers if a user language was not yet defined.
👍 1
konrad marked this conversation as resolved
dpschen force-pushed feature/persist-user-language from 94098a66a6 to 60baf71acf 2021-12-17 17:23:01 +00:00 Compare
konrad commented 2021-12-20 17:45:15 +00:00
Author
Owner
Copy Link

I updated the stuff I was mentioning and force-pushed these changes.

But I found one more issue.
Hope the force-push was fine

I think it's fine, yes. Thanks for fixing the issues :)

So this PR is all done?

> I updated the stuff I was mentioning and force-pushed these changes. > > But I found one more issue. > Hope the force-push was fine I think it's fine, yes. Thanks for fixing the issues :) So this PR is all done?
dpschen was assigned by konrad 2021-12-20 18:38:35 +00:00
dpschen changed title from feat: save and restore the user langauge on the server to feat: save and restore the user language on the server 2021-12-21 11:19:59 +00:00
dpschen requested changes 2021-12-21 11:20:37 +00:00
dpschen left a comment
Member

General:

  • the auth module should probably be renamed to user in the future.
    Or we should create a new store module for this.
  • I think it would be better if the settings would automatically save themselves when you change an option so no need to click save is necessary. What do you think?
General: - the auth module should probably be renamed to user in the future. Or we should create a new store module for this. - I think it would be better if the settings would automatically save themselves when you change an option so no need to click save is necessary. What do you think?
src/store/modules/auth.js
@ -224,0 +224,4 @@
commit('info', info)
commit('lastUserRefresh')
if (typeof info.settings.language !== 'undefined') {
dpschen commented 2021-12-21 11:13:35 +00:00
Member

(I rewrote this myself)

I'm not super happy with this. Especially with the location of this snippet.
As far as I got this this should just be necessary for the first time a user visits the site since after that the setting should not be undefined anymore.

(I rewrote this myself) I'm not super happy with this. Especially with the location of this snippet. As far as I got this this should just be necessary for the first time a user visits the site since after that the setting should not be undefined anymore.
konrad commented 2021-12-26 11:06:48 +00:00
Author
Owner

Yes, but also when the user first visits the site after the new feature was introduced. We might remove this in the future.

Yes, but also when the user first visits the site after the new feature was introduced. We might remove this in the future.
dpschen commented 2021-12-30 13:13:37 +00:00
Member

So should we keep it then as is?

So should we keep it then as is?
konrad commented 2021-12-30 15:01:01 +00:00
Author
Owner

I think yes, for now at least.

I think yes, for now at least.
👍 1
konrad marked this conversation as resolved
src/views/user/settings/General.vue Outdated
@ -171,3 +168,3 @@
userSettingsService: new UserSettingsService(),
settings: {...this.$store.state.auth.settings},
id: createRandomID(),
availableLanguages: Object.entries(availableLanguages)
dpschen commented 2021-12-21 11:15:40 +00:00
Member

Rename to availableLanguageOptions to differentiate from the @/i18n export with the same name.

Rename to `availableLanguageOptions` to differentiate from the `@/i18n` export with the same name.
konrad commented 2021-12-26 11:07:20 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
src/views/user/settings/General.vue Outdated
@ -221,3 +213,2 @@
await this.userSettingsService.update(settings)
this.$store.commit('auth/setUserSettings', settings)
await this.$store.dispatch('auth/saveUserSettings', {...this.settings})
this.$message.success({message: this.$t('user.settings.general.savedSuccess')})
dpschen commented 2021-12-21 11:19:44 +00:00
Member

It feels to me like this message should be in the store with the action. But since we use the same action triggered automatically in the refreshUserInfo this would be confusing since there would be a notification without any interaction.

It feels to me like this message should be in the store with the action. But since we use the same action triggered automatically in the `refreshUserInfo` this would be confusing since there would be a notification without any interaction.
konrad commented 2021-12-26 11:07:57 +00:00
Author
Owner

Definitely. AFAIK triggering the message from the store isn't as easy anyway?

Definitely. AFAIK triggering the message from the store isn't as easy anyway?
dpschen commented 2021-12-30 13:14:41 +00:00
Member

Triggering message is easy. Problem is that this action is also used by the app, in this case we don't want the message to be hidden.

Maybe add a param to the action to hide the message?

Triggering message is easy. Problem is that this action is also used by the app, in this case we don't want the message to be hidden. Maybe add a param to the action to hide the message?
konrad commented 2021-12-30 15:02:07 +00:00
Author
Owner

I think that could work.

How do I trigger a message without access to this.$message? Just by importing the message handler directly?

I think that could work. How do I trigger a message without access to `this.$message`? Just by importing the message handler directly?
dpschen commented 2021-12-30 15:18:37 +00:00
Member

Yes, like here: https://kolaente.dev/vikunja/frontend/src/branch/main/src/store/modules/kanban.js#L356

Yes, like here: https://kolaente.dev/vikunja/frontend/src/branch/main/src/store/modules/kanban.js#L356
👍 1
konrad commented 2021-12-30 15:36:02 +00:00
Author
Owner

Changed.

Changed.
konrad marked this conversation as resolved
konrad was assigned by dpschen 2021-12-21 12:57:58 +00:00
dpschen removed their assignment 2021-12-21 12:58:02 +00:00
konrad added 1 commit 2021-12-26 11:07:17 +00:00
continuous-integration/drone/pr Build is failing Details
a865dcc202
chore: rename available language options
dpschen was assigned by konrad 2021-12-26 11:08:19 +00:00
konrad commented 2021-12-26 11:08:32 +00:00
Author
Owner
Copy Link

@dpschen What else is missing here?

@dpschen What else is missing here?
konrad added 1 commit 2021-12-30 15:35:54 +00:00
continuous-integration/drone/pr Build is failing Details
47d6751a56
chore: trigger success message from store
konrad requested review from dpschen 2021-12-30 15:36:14 +00:00
dpschen approved these changes 2021-12-30 15:42:16 +00:00
konrad added 1 commit 2021-12-30 16:31:23 +00:00
continuous-integration/drone/pr Build is passing Details
09d674cd5d
feat: show success message by default
konrad merged commit 4a7d2d8414 into main 2021-12-30 20:20:46 +00:00
konrad deleted branch feature/persist-user-language 2021-12-30 20:20:46 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
dpschen
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1181
No description provided.
Delete Branch "feature/persist-user-language"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?