feat: manage caldav tokens #1186
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#1186
Loading…
Reference in New Issue
No description provided.
Delete Branch "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?
Frontend PR for vikunja/api#1065
@ -0,0 +6,4 @@
this.created = new Date(this.created)
}
defaults() {
Why does the defaults function exist instead of using field declarations (maybe even private)?
Asking because if it would be:
typing would be better. But maybe I miss something.
I think it only exists because of my lack of js knowledge back a few years ago when I created the models and services :) It should be cleaned up at some point, but for all models at once (which is kinda out of scope for this PR).
I'm pretty sure there's a better way to do this with typescript so that we have proper type hints everywhere.
I think field declarations didn't exist then.
Why I was asking: I'm trying to fix all ts errors that I have while working on a segment. And if creating these props as field declarations helps ts I guess it would be a manageable effort.
Agree with the rest of the stuff =)
@ -25,3 +66,3 @@
</template>
<script>
<script setup>
use ts
@ -30,1 +74,4 @@
import {CALDAV_DOCS} from '@/urls'
import {useTitle} from '@/composables/useTitle'
import {success} from '@/message'
import Message from '../../../components/misc/message'
use
@
@ -31,0 +87,4 @@
const isLocalUser = computed(() => store.state.auth.info?.isLocalUser)
const username = computed(() => store.state.auth.info?.username)
const service = new CaldavTokenService()
Abstract with new
useToken
composable (can be local) that returns the tokens ref.Then the getAll call can also be async (an async init function that is called without await).
How do I properly type hint this? I tried this:
but that does not seem to work.
What errors do you get?
I guess not all props are defined (because I got those kind of errors all the time with the models).
For this the field declarations might help
Seems like
did the trick.
Not sure what problem you had here, but I realize now that it might have also been because of the usage of await in a composable.
@ -31,0 +95,4 @@
})
const newToken = ref(null)
const createToken = () => {
createToken
should be an async function (also defined inuseToken
composable).Done.
Use
function
statement.=> Makes it more clear that these are the "methods" of vue and makes them stand out more against computeds and refs.
Done.
@ -51,3 +106,1 @@
methods: {
copy,
},
const deleteToken = token => {
Should be async function
Done.
Picky: Use function statement.
@ -54,0 +107,4 @@
service.delete(token)
.then(r => {
success(r)
for (const i in tokens.value) {
Use filter
Done.
Now it's
findIndex
but this seems to have other problems, because of the@ts-ignore
.With something like:
It might work =)
On the other hand:
Maybe with that the original solution was nicer / smarter.
I somehow thought tokens is an array and not an object… that's why I proposed using
filter
in the first place.tokens
is an array, not an object. But I need the index of the entry I want to feed intosplice
to remove the entry. AFAIK I can't usefind
because that would only give me the object in the array, not the index?Isn't
findIndex
pretty much the same as afor(const i in ..)
with abreak
?It is if you add a condition for the case of
-1
(which you did now)Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1186-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!
@ -54,0 +108,4 @@
const deleteToken = async (token: CaldavTokenModel) => {
const r = await service.delete(token)
success(r)
// @ts-ignore
Why the
@ts-ignore
? => Add comment if you need itI just took another look and think I figured out how to fix the types on the
useToken
function so the@ts-ignore
isn't required anymore. Should be fine now.Move
success
after all state changesDone.
@ -54,0 +109,4 @@
const r = await service.delete(token)
success(r)
// @ts-ignore
const i = tokens.value.findIndex(v => v.id === token.id)
I think you need to check for
-1
.Done.
@ -54,0 +110,4 @@
success(r)
// @ts-ignore
const i = tokens.value.findIndex(v => v.id === token.id)
// @ts-ignore
Why the
@ts-ignore
? => Add comment if you need it@konrad: Did you see my comments?
@dpschen I did, just didn't have the time to act on them. Will do later today.
@ -97,7 +97,14 @@
"caldav": {
"title": "Caldav",
CalDAV
– different casing. See: https://devguide.calconnect.org/CalDAV/introduction/Done!
@ -97,7 +97,14 @@
"caldav": {
"title": "Caldav",
"howTo": "You can connect Vikunja to caldav clients to view and manage all tasks from different clients. Enter this url into your client:",
casing
@ -98,3 +98,3 @@
"title": "Caldav",
"howTo": "You can connect Vikunja to caldav clients to view and manage all tasks from different clients. Enter this url into your client:",
"more": "More information about caldav in Vikunja"
"more": "More information about caldav in Vikunja",
casing
@ -99,2 +99,3 @@
"howTo": "You can connect Vikunja to caldav clients to view and manage all tasks from different clients. Enter this url into your client:",
"more": "More information about caldav in Vikunja"
"more": "More information about caldav in Vikunja",
"tokens": "Caldav Tokens",
casing
@ -100,1 +100,3 @@
"more": "More information about caldav in Vikunja"
"more": "More information about caldav in Vikunja",
"tokens": "Caldav Tokens",
"tokensHowTo": "You can use a caldav token to use instead of a password to log in the above endpoint.",
casing
@ -101,0 +103,4 @@
"createToken": "Create a token",
"tokenCreated": "Here is your token: {token}",
"wontSeeItAgain": "Write it down, you won't be able to see it again.",
"mustUseToken": "You need to create a caldav token if you want to use caldav with a third party client. Use the token as the password.",
casing
@ -0,0 +12,4 @@
}
processModel(model) {
model.created = formatISO(new Date(model.created))
Return a new model instead of modifying the existing:
Done.
@ -17,2 +17,4 @@
</div>
</div>
<p class="my-4 has-text-weight-bold">
This seems to be a headline? => use
<hX>
@ -31,0 +78,4 @@
import CaldavTokenService from '@/services/caldavToken'
import CaldavTokenModel from '@/models/caldavToken'
const service = new CaldavTokenService()
useTokens
function.shallowReactive
. => Will be destroyed when component is unmounted.serviceIsLoading
fromuseTokens
that computes the isLoading property.=> isolated abstraction
Wouldn't I need to do this for all other functions as well? (
deleteToken
andcreateToken
) How would I handle loading state from all of them? I'd like to avoid having a bunch of loading variables for each of these functions.Solved differently in #1307/
@ -31,0 +80,4 @@
const service = new CaldavTokenService()
async function useToken(): ref<CaldavTokenModel[]> {
We should call this function then
useTokens
.Unsure:
I think
: ref<CaldavTokenModel[]>
is now unnecessary since the type of the return was already defined by the line:Trying to learn: Not sure what the go-to way for ts is in such a case.
Renamed the function.
I'm not quite sure about the other one, since the
const token
now does not have a type declaration, only the function does. I think that should be fine?I cannot follow you:
In this line the type of the returned value is defined.
Regardless this is not important anymore, since the code changed.
@ -31,0 +82,4 @@
async function useToken(): ref<CaldavTokenModel[]> {
const tokens = ref<CaldavTokenModel[]>([])
tokens.value = await service.getAll()
A composable should usually not return a promise. See https://antfu.me/posts/async-with-composition-api
(only exception as far as I know seems to be when you use the
<suspense>
component)Better: create a new function inside the composable that does the async action and call it without await.
Something like:
EDIT:
There is also: useAsyncState
But afaik I need an
async
function when usingawait
? Or use.then()
?Using the example you sent gives me an error about that.
Okay so this does not seem to work at all right now.
I think I got it working in
ef69fd28b2
but I'm not sure if that's a good way to do it. I think I'm returning the ref before it has data in it, that only gets populated once the callback in.then
is done and puts it intokens.value
. Not sure if that's a good way to do it?Continuing this is #1307/
@ -31,0 +101,4 @@
const newToken = ref(null)
async function createToken() {
const r = await service.create({})
Would there be any negative side effect if you would assign to
newToken.value
directly?If yes (picky): I prefere to give api returns a name that makes clear what they contain, e.g.
token
.That seems to work fine, I've changed it accordingly.
@dpschen Could you check again?
f867195af5
toce479b4076
@konrad Can we close this in favour of #1307/ ?
I went through all the issues. Then we just have to merge one pull-request.
I think we could do that.
Who should continue in #1307?