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

feat: improve user assignments via quick add magic #3348

Merged
konrad merged 18 commits from feature/better-user-search-assignees into main 2023-06-05 15:03:16 +00:00
Conversation 6 Commits 18 Files Changed 8 +66 -32
konrad commented 2023-04-03 17:42:51 +00:00
Owner
Copy Link

Resolves #2196

This PR follows the api changes from a7231e197e and ensures users are actually assignable when assigning them via their email. It also clarifies the api changes in the user settings.

Resolves #2196 This PR follows the api changes from https://kolaente.dev/vikunja/api/commit/a7231e197e3d86d3ef27fad89ae60863d25b5df0 and ensures users are actually assignable when assigning them via their email. It also clarifies the api changes in the user settings.
konrad added 4 commits 2023-04-03 17:42:51 +00:00
konrad requested review from dpschen 2023-04-03 17:43:18 +00:00
konrad added 1 commit 2023-04-03 17:45:30 +00:00
konrad added 1 commit 2023-04-03 17:49:50 +00:00
continuous-integration/drone/pr Build is failing Details
dec557eb1d
feat(assignees): show user avatar in search results
konrad added 1 commit 2023-04-03 17:55:59 +00:00
continuous-integration/drone/pr Build is passing Details
c47a0bab19
fix: lint
frederick commented 2023-04-03 17:59:50 +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://3348-feature-better-user-search-assig--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://3348-feature-better-user-search-assig--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.
xeruf commented 2023-04-03 20:13:38 +00:00
First-time contributor
Copy Link

Something is borked here:
I create a task named go @demo which is assigned to demo, but the title ends up with go demo

Something is borked here: I create a task named `go @demo` which is assigned to demo, but the title ends up with `go demo`
xeruf commented 2023-04-03 20:25:59 +00:00
First-time contributor
Copy Link

yes, this happens for assignment by username and displayname, but not per email

yes, this happens for assignment by username and displayname, but not per email
xeruf commented 2023-04-03 20:37:10 +00:00
First-time contributor
Copy Link

user discovery and email assignment seem fine

user discovery and email assignment seem fine
dpschen commented 2023-04-03 20:43:40 +00:00
Member
Copy Link

yes, this happens for assignment by username and displayname, but not per email

When we switch the editor this will be simpler to implement.

> yes, this happens for assignment by username and displayname, but not per email When we switch the editor this will be simpler to implement.
konrad added 1 commit 2023-04-04 08:41:38 +00:00
konrad commented 2023-04-04 08:42:21 +00:00
Author
Owner
Copy Link

Something is borked here:
I create a task named go @demo which is assigned to demo, but the title ends up with go demo

This should now be fixed.

> Something is borked here: > I create a task named `go @demo` which is assigned to demo, but the title ends up with `go demo` This should now be fixed.
konrad added 1 commit 2023-04-04 09:05:59 +00:00
konrad removed review request for dpschen 2023-04-10 17:37:53 +00:00
konrad requested review from dpschen 2023-04-10 17:37:57 +00:00
dpschen requested changes 2023-04-11 09:45:27 +00:00
src/components/tasks/partials/editAssignees.vue Outdated
@ -19,2 +19,4 @@
</span>
</template>
<template #searchResult="{option: user}">
<user :avatar-size="24" :show-username="true" :user="user" class="user-search-result"/>
dpschen commented 2023-04-03 20:56:24 +00:00
Member

Use a prop here and change user so that it doesn't add a margin.

Reason: If we want to remove the margin in general from the user component later (which we should and I think I have done that already in some branch) then it will be much easier if we don't have classes that have dependencies that we are not aware of.

Sum up:

  • setting margin from outside of components is fine
  • unsetting margin from outside if there is already one from inside (which shouldn't be there in the first place) is not fine.
Use a prop here and change user so that it doesn't add a margin. Reason: If we want to remove the margin in general from the user component later (which we should and I think I have done that already in some branch) then it will be much easier if we don't have classes that have dependencies that we are not aware of. Sum up: - setting margin from outside of components is fine - unsetting margin from outside if there is already one from inside (which shouldn't be there in the first place) is not fine.
✅ 1
konrad commented 2023-04-11 17:22:48 +00:00
Author
Owner

My first gut-reaction was, "but the user component is used a lot! That's quite a bit of effort to change it everywhere!". Then I started looking into and as it turns out, there are only 3 places where the margin from the component is not overridden from the outer component. That made it easier and kind of proved your point :)

My first gut-reaction was, "but the user component is used a lot! That's quite a bit of effort to change it everywhere!". Then I started looking into and as it turns out, there are only 3 places where the margin from the component _is not_ overridden from the outer component. That made it easier and kind of proved your point :)
😆 1
dpschen marked this conversation as resolved
src/views/user/settings/General.vue
@ -48,12 +48,18 @@
<input type="checkbox" v-model="settings.discoverableByName"/>
{{ $t('user.settings.general.discoverableByName') }}
</label>
dpschen commented 2023-04-03 21:47:38 +00:00
Member

By discoverable you mean "able to invite"?
Because I can't assign someone if that person isn't part of the current project, correct?

In general I was now quite confused what this setting now actually does. Judging from this most users will probably not have a clue.

By `discoverable` you mean "able to invite"? Because I can't assign someone if that person isn't part of the current project, correct? In general I was now quite confused what this setting now actually does. Judging from this most users will probably not have a clue.
✅ 1
konrad commented 2023-04-11 17:26:21 +00:00
Author
Owner

That's correct! Before the recent changes about finding users in the api, this setting would apply to searching users with access to a project as well. Now it does not anymore and the little explanation I added below does not make this clearer. I've adjusted the message and removed the explanation below the setting.

That's correct! Before the recent changes about finding users in the api, this setting would apply to searching users with access to a project as well. Now it does not anymore and the little explanation I added below does not make this clearer. I've adjusted the message and removed the explanation below the setting.
dpschen commented 2023-04-12 13:14:59 +00:00
Member
  • Do you mean 'mention' by 'add me', because I'm still confused what 'search' in that context means?
  • "Allow other users to invite me to teams or projects."
  • group under 'Privacy'? This would add some context to these two settings.
  • Is there ever a case where someone would like to enable / disable only one of those settings? If not: should we group them? If not on api side, maybe set both settings at once in frontend.
- Do you mean 'mention' by 'add me', because I'm still confused what 'search' in that context means? - "Allow other users to **invite** me to teams or projects." - group under 'Privacy'? This would add some context to these two settings. - Is there ever a case where someone would like to enable / disable only one of those settings? If not: should we group them? If not on api side, maybe set both settings at once in frontend.
konrad commented 2023-04-12 16:41:36 +00:00
Author
Owner

Do you mean 'mention' by 'add me', because I'm still confused what 'search' in that context means?

No, it's literally adding a user to a project or team. Mentioning is a topic for another day.

"Allow other users to invite me to teams or projects."

Inviting implies there's a form of "receiving an invitation and be able to reject it" which is not the case right now. There is an item on the roadmap to change that, but that's very much out of scope here (and it requires quite a few changes in the api).

group under 'Privacy'? This would add some context to these two settings.

We absolutely should group these settings more. Right now there's too much stuff and it does not look great. I'd prefer to move that to another PR since it would touch more than these two options and I feel like we should plan this properly first.

Is there ever a case where someone would like to enable / disable only one of those settings? If not: should we group them? If not on api side, maybe set both settings at once in frontend.

I guess that depends on how common your name is :) I think it makes sense to have them as distinct options, because an email address is unique, but a name might not be. Searching for a name also "feels" more personal than allowing someone else to find me by my email address.

> Do you mean 'mention' by 'add me', because I'm still confused what 'search' in that context means? No, it's literally adding a user to a project or team. Mentioning is a topic for another day. > "Allow other users to invite me to teams or projects." Inviting implies there's a form of "receiving an invitation and be able to reject it" which is not the case right now. There is an item on the roadmap to change that, but that's very much out of scope here (and it requires quite a few changes in the api). > group under 'Privacy'? This would add some context to these two settings. We absolutely should group these settings more. Right now there's too much stuff and it does not look great. I'd prefer to move that to another PR since it would touch more than these two options and I feel like we should plan this properly first. > Is there ever a case where someone would like to enable / disable only one of those settings? If not: should we group them? If not on api side, maybe set both settings at once in frontend. I guess that depends on how common your name is :) I think it makes sense to have them as distinct options, because an email address is unique, but a name might not be. Searching for a name also "feels" more personal than allowing someone else to find me by my email address.
dpschen commented 2023-04-12 18:30:53 +00:00
Member

adding a user to a project

How about 'add member' then?

I'd prefer to move that to another PR

makes sense

keep splitting options

Makes sense as well

> adding a user to a project How about 'add member' then? > I'd prefer to move that to another PR makes sense *keep splitting options* Makes sense as well
👍 1
konrad commented 2023-04-14 09:10:36 +00:00
Author
Owner

I've changed the string yet again

I've changed the string yet again
👍 1
dpschen marked this conversation as resolved
src/views/user/settings/General.vue Outdated
@ -48,12 +48,18 @@
<input type="checkbox" v-model="settings.discoverableByName"/>
{{ $t('user.settings.general.discoverableByName') }}
</label>
<p class="is-size-7" v-if="!settings.discoverableByName">
dpschen commented 2023-04-03 20:50:20 +00:00
Member

Wrap test inside <p> with <small>.

Wrap test inside `<p>` with [`<small>`](https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-small-element).
✅ 1
dpschen marked this conversation as resolved
src/views/user/settings/General.vue Outdated
@ -54,3 +57,4 @@
<input type="checkbox" v-model="settings.discoverableByEmail"/>
{{ $t('user.settings.general.discoverableByEmail') }}
</label>
<p class="is-size-7" v-if="!settings.discoverableByEmail">
dpschen commented 2023-04-03 20:52:24 +00:00
Member

Wrap test inside <p> with <small>.

Wrap test inside `<p>` with [`<small>`](https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-small-element).
✅ 1
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-11 11:03:00 +00:00
src/stores/tasks.ts
@ -32,2 +34,4 @@
match: string,
}
// IDEA: maybe use a small fuzzy search here to prevent errors
dpschen commented 2023-04-11 11:02:59 +00:00
Member

For a simple implementation copy over: https://github.com/hiddentao/fast-levenshtein/blob/3.0.0/levenshtein.js

For a simple implementation copy over: https://github.com/hiddentao/fast-levenshtein/blob/3.0.0/levenshtein.js
✅ 1
konrad commented 2023-04-11 17:16:09 +00:00
Author
Owner

Why copy it instead of pulling it in as a dependency?

We don't do any fancy matching in the api (other than lowercase contains), which means the frontend can't find things with fuzzy matching which the api did not return in the first place. I feel like we should fuzzy matching in the api first and then add it in the frontend as well.

Why copy it instead of pulling it in as a dependency? We don't do any fancy matching in the api (other than lowercase contains), which means the frontend can't find things with fuzzy matching which the api did not return in the first place. I feel like we should fuzzy matching in the api first and then add it in the frontend as well.
dpschen commented 2023-04-11 20:23:16 +00:00
Member

The module is a slim wrapper around fastest-levenshtein with some sensible defaults and using Intl.Collator. The wrapper is a bit dated. So I would only copy the function as a helper and import fastest-levenshtein directly. Fuzzy matching in the api makes sense.

Doing it in the frontend as well for the locally available data still makes sense, because fuzzy finding increases the chance that the wanted result is further at the top in the list of found results.

This is a really cheap method with a big impact. For the backend I would probably try to use something better directly.

The module is a slim wrapper around [fastest-levenshtein](https://github.com/ka-weihe/fastest-levenshtein) with some sensible defaults and using [Intl.Collator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Collator). The wrapper is a bit dated. So I would only copy the function as a helper and import fastest-levenshtein directly. Fuzzy matching in the api makes sense. Doing it in the frontend as well for the locally available data still makes sense, because fuzzy finding increases the chance that the wanted result is further at the top in the list of found results. This is a really cheap method with a big impact. For the backend I would probably try to use something better directly.
konrad commented 2023-04-12 07:45:17 +00:00
Author
Owner

But if the frontend and api both do fuzzy matching, what's the advantage of doing it in the frontend as well? Wouldn't that give use the same results? (right now neither does, but it should be done in the api anyway)

But if the frontend and api both do fuzzy matching, what's the advantage of doing it in the frontend as well? Wouldn't that give use the same results? (right now neither does, but it should be done in the api anyway)
dpschen commented 2023-04-12 12:06:46 +00:00
Member

We have a few points in the code where we used strict matching for searching in local (aka downloaded to frontend) data. I thought this was one of those places. Since adding levenshtein seems simple I thought we would have a quick-win here. If we are offline first this also enables usage when offline without server request. To get up-to-date data an api-request is obviously unavoidable. So I'm unsure what our general strategy here is, I guess?

We have a few points in the code where we used strict matching for searching in local (aka downloaded to frontend) data. I thought this was one of those places. Since adding levenshtein seems simple I thought we would have a quick-win here. If we are offline first this also enables usage when offline without server request. To get up-to-date data an api-request is obviously unavoidable. So I'm unsure what our general strategy here is, I guess?
konrad commented 2023-04-12 16:34:28 +00:00
Author
Owner

Sure, we could add it but I'm not sure how useful that would be in the current state. The offline argument sticks though… but then we could do that when we have offline storage. Better not complicate things more than we must right now to solve this.

Sure, we could add it but I'm not sure how useful that would be in the current state. The offline argument sticks though… but then we could do that when we have offline storage. Better not complicate things more than we must right now to solve this.
👍 1
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-11 17:21:00 +00:00
continuous-integration/drone/pr Build is passing Details
a8e1f60e81
chore: remove user margin from the component
konrad added 1 commit 2023-04-11 17:21:51 +00:00
continuous-integration/drone/pr Build is passing Details
4d55a5b79d
chore: remove user margin from the component
konrad added 1 commit 2023-04-11 17:26:37 +00:00
continuous-integration/drone/pr Build is passing Details
45e9376330
fix: clarify user search setting
konrad removed review request for dpschen 2023-04-11 17:27:00 +00:00
konrad requested review from dpschen 2023-04-11 17:27:02 +00:00
dpschen reviewed 2023-04-11 21:53:10 +00:00
src/modules/parseTaskText.ts Outdated
@ -110,3 +110,3 @@
}
p = p.replace(prefix, '')
if (p.substring(0, 1) === prefix) {
dpschen commented 2023-04-11 21:53:10 +00:00
Member

Picky: use startsWith

Picky: use `startsWith`
✅ 1
konrad commented 2023-04-12 07:47:01 +00:00
Author
Owner

Done

Done
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-12 07:47:25 +00:00
continuous-integration/drone/pr Build is passing Details
fc7862b837
chore: use startsWith for prefix matching
dpschen reviewed 2023-04-12 12:59:01 +00:00
src/modules/parseTaskText.ts Outdated
@ -278,13 +280,16 @@ const getRepeats = (text: string): repeatParsedResult => {
export const cleanupItemText = (text: string, items: string[], prefix: string): string => {
dpschen commented 2023-04-12 12:56:31 +00:00
Member

Would this improve this function:

import { createRegExp, maybe, groupedAs } from 'magic-regexp';

/**
 * Removes the given items from the text.
 * @param text The input text.
 * @param items The items to remove from the text.
 * @param prefix The prefix character to use when matching items in the text. Default is an empty string.
 * @returns The modified text.
 */
function cleanupItemText(text: string, items: string[], prefix = ''): string {
  // Escape special characters in the items and join them with the `|` character to form a regex character class.
  const escapedItems = items.map((item) => createRegExp(item).escape());
  const itemPattern = createRegExp(maybe(prefix)).and.anyOf(escapedItems).grouped().source;
  const pattern = createRegExp(itemPattern, 'gi');
  // Replace all occurrences of the pattern in the text with an empty string.
  return text.replace(pattern, '');
}
Would this improve this function: ```ts import { createRegExp, maybe, groupedAs } from 'magic-regexp'; /** * Removes the given items from the text. * @param text The input text. * @param items The items to remove from the text. * @param prefix The prefix character to use when matching items in the text. Default is an empty string. * @returns The modified text. */ function cleanupItemText(text: string, items: string[], prefix = ''): string { // Escape special characters in the items and join them with the `|` character to form a regex character class. const escapedItems = items.map((item) => createRegExp(item).escape()); const itemPattern = createRegExp(maybe(prefix)).and.anyOf(escapedItems).grouped().source; const pattern = createRegExp(itemPattern, 'gi'); // Replace all occurrences of the pattern in the text with an empty string. return text.replace(pattern, ''); } ```
✅ 1
konrad commented 2023-04-12 16:36:17 +00:00
Author
Owner

I don't think it would improve it by much. IMHO the current version is easier to understand what it does.

I don't think it would improve it by much. IMHO the current version is easier to understand what it does.
👍 1
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-14 09:10:26 +00:00
continuous-integration/drone/pr Build is passing Details
f64eacafba
chore(i18n): clarify translation string
dpschen reviewed 2023-04-14 09:45:09 +00:00
src/stores/tasks.ts Outdated
@ -31,1 +30,4 @@
import {useBaseStore} from '@/stores/base'
import ProjectUserService from '@/services/projectUsers'
interface MatchedAssignees extends IUser {
dpschen commented 2023-04-14 09:45:09 +00:00
Member

This should be singular, like IUser

This should be singular, like `IUser`
konrad commented 2023-04-14 13:34:30 +00:00
Author
Owner

Done

Done
konrad marked this conversation as resolved
dpschen reviewed 2023-04-14 09:48:00 +00:00
src/stores/tasks.ts Outdated
@ -45,1 +58,4 @@
findPropertyByValueFuzzy(users, 'email', query)
}
return findPropertyByValue(users, 'username', query) ||
dpschen commented 2023-04-14 09:48:00 +00:00
Member

Fuzzy should be an option of findPropertyByValue.
Picky: use bracket to group return value.

return (
	findPropertyByValue(users, 'username', query) ||
	findPropertyByValue(users, 'name', query) ||
	findPropertyByValue(users, 'email', query)
)
Fuzzy should be an option of `findPropertyByValue`. Picky: use bracket to group return value. ```ts return ( findPropertyByValue(users, 'username', query) || findPropertyByValue(users, 'name', query) || findPropertyByValue(users, 'email', query) )
konrad commented 2023-04-14 13:37:14 +00:00
Author
Owner

Done

Done
konrad marked this conversation as resolved
dpschen reviewed 2023-04-14 09:49:26 +00:00
src/stores/tasks.ts Outdated
@ -72,2 +87,3 @@
const userService = new ProjectUserService()
const assignees = parsedTaskAssignees.map(async a => {
const users = await userService.getAll({}, {s: a})
const users = (await userService.getAll({projectId}, {s: a}))
dpschen commented 2023-04-14 09:49:26 +00:00
Member

The bracket around seems to be unnecessary.

What I meant: Seems better to use await with then here

~~The bracket around seems to be unnecessary.~~ What I meant: Seems better to use `await` with `then` here
konrad commented 2023-04-14 13:37:52 +00:00
Author
Owner

Don't you mean then without await?

Don't you mean `then` without `await`?
dpschen commented 2023-04-14 13:45:23 +00:00
Member

No. You still need to await the resulting promise comining out of the then

No. You still need to await the resulting promise comining out of the then
konrad commented 2023-04-14 13:50:21 +00:00
Author
Owner

Then what's the advantage of the extra then over the current solution?

Then what's the advantage of the extra `then` over the current solution?
dpschen reviewed 2023-04-14 11:42:00 +00:00
src/stores/tasks.ts Outdated
@ -392,2 +411,2 @@
const assignees = await findAssignees(parsedTask.assignees)
const assignees = await findAssignees(parsedTask.assignees, foundProjectId)
dpschen commented 2023-04-14 11:42:00 +00:00
Member

assignees is used to create a task via new TaskModel. findAssignees now returns an array of MatchedAssignee(s) instead of IUser[] which is what new TaskModel expects as type for the assignees property

`assignees` is used to create a task via `new TaskModel`. `findAssignees` now returns an array of `MatchedAssignee`(`s`) instead of `IUser[]` which is what `new TaskModel` expects as type for the `assignees` property
konrad commented 2023-04-14 13:40:15 +00:00
Author
Owner

That should not be a problem, because MatchedAssignee implements IUser and thus can be used as such. If it can quack like a duck, it can be used as a duck, no matter what else it is capable of doing.
The match property of MatchedAssignee will be ignored by the model (from the type perspective, in reality it will be sent to the api which will then ignore it).

That should not be a problem, because `MatchedAssignee` implements `IUser` and thus can be used as such. If it can quack like a duck, it can be used as a duck, no matter what else it is capable of doing. The `match` property of `MatchedAssignee` will be ignored by the model (from the type perspective, in reality it will be sent to the api which will then ignore it).
konrad added 2 commits 2023-04-14 13:36:42 +00:00
continuous-integration/drone/pr Build is failing Details
d55ab01c75
chore: make fuzzy matching a paramater
konrad added 1 commit 2023-04-14 13:37:11 +00:00
continuous-integration/drone/pr Build is failing Details
b721dc2d34
chore: group return parameter
konrad force-pushed feature/better-user-search-assignees from b721dc2d34 to 60e051433c 2023-04-25 16:33:55 +00:00 Compare
konrad force-pushed feature/better-user-search-assignees from 60e051433c to d1025c3982 2023-04-26 16:40:40 +00:00 Compare
konrad force-pushed feature/better-user-search-assignees from d1025c3982 to 4aad488707 2023-05-31 14:19:01 +00:00 Compare
konrad added 1 commit 2023-05-31 14:22:05 +00:00
continuous-integration/drone/pr Build was killed Details
7e50b6e270
fix: lint
konrad force-pushed feature/better-user-search-assignees from 7e50b6e270 to cd2b7fe185 2023-06-05 14:09:21 +00:00 Compare
konrad merged commit d9f608e8b4 into main 2023-06-05 15:03:16 +00:00
konrad deleted branch feature/better-user-search-assignees 2023-06-05 15:03:16 +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
4 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#3348
No description provided.
Delete Branch "feature/better-user-search-assignees"

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?