feat: manage caldav tokens [addition] #1307
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1307
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/caldav-tokens"
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?
Based on #1186.
Didn't want to push to the original branch without sharing first.
I also do not currently know how I could test this.
TODO:
CalDAV
in whole requestHi dpschen!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1307-featurecaldav-tokens--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!
Using ts here improves the DX a lot.
I also started converting the abstractService to ts. But I had too many issues with it so removed it again.
What I currently have:
There is a lot of stuff where I'm not sure how to do this correctly. To give an example:
AbstractModel
as type, but that obviously doesn't work.I had some progress with this, but was not able to succeed yet.
Since this has nothing to do with the pull request I'm resolving this.
@ -0,0 +1,14 @@
import AbstractModel from './abstractModel'
export default class CaldavTokenModel extends AbstractModel {
id = 0
Since
AbstractModel
already defines adefaults
function we don't have to implement one here.For better ts support we can define fields
@ -0,0 +2,4 @@
import CaldavTokenModel from '../models/caldavToken'
import AbstractService from './abstractService'
export default class CaldavTokenService extends AbstractService {
I think we should also use the correct casing here (file and class name). What do you think?
Sounds like a good idea.
@ -19,2 +59,3 @@
<p>
<a href="https://vikunja.io/docs/caldav/" rel="noreferrer noopener nofollow" target="_blank">
<BaseButton :href="CALDAV_DOCS" rel="noreferrer noopener nofollow" target="_blank">
In case
<BaseButton>
is a link (not arouter-link
):Do you think we always want
rel="noreferrer noopener nofollow"
to be set?The current default
rel
for links is onlynoopener
(because copied from my own code).Yeah, that sounds like a good idea.
Solved in #1491
@ -31,0 +79,4 @@
import CaldavTokenService from '@/services/caldavToken'
import CaldavTokenModel from '@/models/caldavToken'
const {t} = useI18n()
I think we should always start the view components with the title composable.
Sometimes data fetching might need to be first of course.
@ -31,0 +82,4 @@
const {t} = useI18n()
useTitle(() => `${t('user.settings.caldav.title')} - ${t('user.settings.title')}`)
const service = shallowReactive(new CaldavTokenService())
I think (not sure) that using some kind of reactive for stuff like this is a good pattern, because this way you can always extract parts in a composable if needed later. Often we want to do this after a component has grown in size because some stuff was added.
@ -31,0 +85,4 @@
const service = shallowReactive(new CaldavTokenService())
const tokens = ref<CaldavTokenModel[]>([])
service.getAll().then((result: CaldavTokenModel[]) => {
I decided to flatten this again. Sorry for this… I'm also still lerning the composition api =)
Originally I thought that this might be reused in other components.
@ -55,0 +97,4 @@
async function deleteToken(token: CaldavTokenModel) {
const r = await service.delete(token)
tokens.value = tokens.value.filter(({id}) => id !== token.id)
Not sure if this works aswell.
From vue's perspective filtering and reassigning should have almost the same speed.
But I was not sure if this makes sense, since I am still calling success even if tokens stays the same after filtering because the condition never succeded.
But wouldn't the condition only never succeed when the token which was deleted is not present in the list? That case should never happen. Because of this, I think showing the success message regardless is not wrong here.
@ -55,0 +103,4 @@
const store = useStore()
const username = computed(() => store.state.auth.info?.username)
const caldavUrl = computed(() => `${store.getters['config/apiBase']}/dav/principals/${username.value}/`)
I reused the
username
computed hereWIP: feat: manage caldav tokens [addition]to feat: manage caldav tokens [addition]Maybe merging #1186 first could be a good idea? Would make this one easier to review at least
In theory this is the diff
e2e0a6576c
toa221b24ccf
Fix this: #1489 (comment)
@ -103,9 +103,16 @@
"disableSuccess": "Two factor authentication was sucessfully disabled."
},
"caldav": {
Is it possible to change this translation id?
Can you make sure every string is shown correctly after changing it? Maybe not in this PR?
e4919ca00d
tod014174011
d014174011
to8eba71d448
90e47905b1
to58b0397cec
Should be fine to merge now with the fixes.
Thanks!