feat: improve user assignments via quick add magic #3348
|
@ -48,7 +48,6 @@ const displayName = computed(() => getDisplayName(props.user))
|
||||||
|
|
||||||
<style lang="scss" scoped>
|
<style lang="scss" scoped>
|
||||||
.user {
|
.user {
|
||||||
margin: .5rem;
|
|
||||||
display: flex;
|
display: flex;
|
||||||
justify-items: center;
|
justify-items: center;
|
||||||
|
|
||||||
|
|
|
@ -20,7 +20,8 @@
|
||||||
:user="n.notification.doer"
|
:user="n.notification.doer"
|
||||||
:show-username="false"
|
:show-username="false"
|
||||||
:avatar-size="16"
|
:avatar-size="16"
|
||||||
v-if="n.notification.doer"/>
|
v-if="n.notification.doer"
|
||||||
|
/>
|
||||||
<div class="detail">
|
<div class="detail">
|
||||||
<div>
|
<div>
|
||||||
<span class="has-text-weight-bold mr-1" v-if="n.notification.doer">
|
<span class="has-text-weight-bold mr-1" v-if="n.notification.doer">
|
||||||
|
|
|
@ -12,12 +12,15 @@
|
||||||
>
|
>
|
||||||
<template #tag="{item: user}">
|
<template #tag="{item: user}">
|
||||||
<span class="assignee">
|
<span class="assignee">
|
||||||
<user :avatar-size="32" :show-username="false" :user="user"/>
|
<user :avatar-size="32" :show-username="false" :user="user" class="m-2"/>
|
||||||
<BaseButton @click="removeAssignee(user)" class="remove-assignee" v-if="!disabled">
|
<BaseButton @click="removeAssignee(user)" class="remove-assignee" v-if="!disabled">
|
||||||
<icon icon="times"/>
|
<icon icon="times"/>
|
||||||
</BaseButton>
|
</BaseButton>
|
||||||
</span>
|
</span>
|
||||||
</template>
|
</template>
|
||||||
|
<template #searchResult="{option: user}">
|
||||||
|
<user :avatar-size="24" :show-username="true" :user="user"/>
|
||||||
dpschen marked this conversation as resolved
Outdated
|
|||||||
|
</template>
|
||||||
</Multiselect>
|
</Multiselect>
|
||||||
</template>
|
</template>
|
||||||
|
|
||||||
|
@ -104,11 +107,6 @@ async function removeAssignee(user: IUser) {
|
||||||
}
|
}
|
||||||
|
|
||||||
async function findUser(query: string) {
|
async function findUser(query: string) {
|
||||||
if (query === '') {
|
|
||||||
foundUsers.value = []
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
const response = await projectUserService.getAll({projectId: props.projectId}, {s: query}) as IUser[]
|
const response = await projectUserService.getAll({projectId: props.projectId}, {s: query}) as IUser[]
|
||||||
|
|
||||||
// Filter the results to not include users who are already assigned
|
// Filter the results to not include users who are already assigned
|
||||||
|
|
|
@ -56,6 +56,7 @@
|
||||||
:key="task.id + 'assignee' + a.id + i"
|
:key="task.id + 'assignee' + a.id + i"
|
||||||
:show-username="false"
|
:show-username="false"
|
||||||
:user="a"
|
:user="a"
|
||||||
|
class="m-2"
|
||||||
/>
|
/>
|
||||||
|
|
||||||
<!-- FIXME: use popup -->
|
<!-- FIXME: use popup -->
|
||||||
|
|
|
@ -76,8 +76,8 @@
|
||||||
"savedSuccess": "The settings were successfully updated.",
|
"savedSuccess": "The settings were successfully updated.",
|
||||||
"emailReminders": "Send me reminders for tasks via Email",
|
"emailReminders": "Send me reminders for tasks via Email",
|
||||||
"overdueReminders": "Send me a summary of my undone overdue tasks every day",
|
"overdueReminders": "Send me a summary of my undone overdue tasks every day",
|
||||||
"discoverableByName": "Let other users find me when they search for my name",
|
"discoverableByName": "Allow other users to add me as a member to teams or projects when they search for my name",
|
||||||
"discoverableByEmail": "Let other users find me when they search for my full email",
|
"discoverableByEmail": "Allow other users to add me as a member to teams or projects when they search for my full email",
|
||||||
"playSoundWhenDone": "Play a sound when marking tasks as done",
|
"playSoundWhenDone": "Play a sound when marking tasks as done",
|
||||||
"weekStart": "Week starts on",
|
"weekStart": "Week starts on",
|
||||||
"weekStartSunday": "Sunday",
|
"weekStartSunday": "Sunday",
|
||||||
|
|
|
@ -691,6 +691,14 @@ describe('Parse Task Text', () => {
|
||||||
expect(result.assignees).toHaveLength(1)
|
expect(result.assignees).toHaveLength(1)
|
||||||
expect(result.assignees[0]).toBe('today')
|
expect(result.assignees[0]).toBe('today')
|
||||||
})
|
})
|
||||||
|
it('should recognize an email address', () => {
|
||||||
|
const text = 'Lorem Ipsum @email@example.com'
|
||||||
|
const result = parseTaskText(text)
|
||||||
|
|
||||||
|
expect(result.text).toBe('Lorem Ipsum @email@example.com')
|
||||||
|
expect(result.assignees).toHaveLength(1)
|
||||||
|
expect(result.assignees[0]).toBe('email@example.com')
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
describe('Recurring Dates', () => {
|
describe('Recurring Dates', () => {
|
||||||
|
|
|
@ -109,7 +109,9 @@ const getItemsFromPrefix = (text: string, prefix: string): string[] => {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
p = p.replace(prefix, '')
|
if (p.startsWith(prefix)) {
|
||||||
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Picky: use Picky: use `startsWith`
konrad
commented
Done Done
|
|||||||
|
p = p.substring(1)
|
||||||
|
}
|
||||||
|
|
||||||
let itemText
|
let itemText
|
||||||
if (p.charAt(0) === '\'') {
|
if (p.charAt(0) === '\'') {
|
||||||
|
@ -120,8 +122,8 @@ const getItemsFromPrefix = (text: string, prefix: string): string[] => {
|
||||||
// Only until the next space
|
// Only until the next space
|
||||||
itemText = p.split(' ')[0]
|
itemText = p.split(' ')[0]
|
||||||
}
|
}
|
||||||
|
|
||||||
if(itemText !== '') {
|
if (itemText !== '') {
|
||||||
items.push(itemText)
|
items.push(itemText)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
@ -278,13 +280,16 @@ const getRepeats = (text: string): repeatParsedResult => {
|
||||||
|
|
||||||
export const cleanupItemText = (text: string, items: string[], prefix: string): string => {
|
export const cleanupItemText = (text: string, items: string[], prefix: string): string => {
|
||||||
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Would this improve this function:
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, '');
}
```
konrad
commented
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.
|
|||||||
items.forEach(l => {
|
items.forEach(l => {
|
||||||
|
if (l === '') {
|
||||||
|
return
|
||||||
|
}
|
||||||
text = text
|
text = text
|
||||||
.replace(`${prefix}'${l}' `, '')
|
.replace(new RegExp(`\\${prefix}'${l}' `, 'ig'), '')
|
||||||
.replace(`${prefix}'${l}'`, '')
|
.replace(new RegExp(`\\${prefix}'${l}'`, 'ig'), '')
|
||||||
.replace(`${prefix}"${l}" `, '')
|
.replace(new RegExp(`\\${prefix}"${l}" `, 'ig'), '')
|
||||||
.replace(`${prefix}"${l}"`, '')
|
.replace(new RegExp(`\\${prefix}"${l}"`, 'ig'), '')
|
||||||
.replace(`${prefix}${l} `, '')
|
.replace(new RegExp(`\\${prefix}${l} `, 'ig'), '')
|
||||||
.replace(`${prefix}${l}`, '')
|
.replace(new RegExp(`\\${prefix}${l}`, 'ig'), '')
|
||||||
})
|
})
|
||||||
return text
|
return text
|
||||||
}
|
}
|
||||||
|
|
|
@ -5,7 +5,6 @@ import router from '@/router'
|
||||||
import TaskService from '@/services/task'
|
import TaskService from '@/services/task'
|
||||||
import TaskAssigneeService from '@/services/taskAssignee'
|
import TaskAssigneeService from '@/services/taskAssignee'
|
||||||
import LabelTaskService from '@/services/labelTask'
|
import LabelTaskService from '@/services/labelTask'
|
||||||
import UserService from '@/services/user'
|
|
||||||
|
|
||||||
import {playPop} from '@/helpers/playPop'
|
import {playPop} from '@/helpers/playPop'
|
||||||
import {getQuickAddMagicMode} from '@/helpers/quickAddMagicMode'
|
import {getQuickAddMagicMode} from '@/helpers/quickAddMagicMode'
|
||||||
|
@ -29,12 +28,21 @@ import {useProjectStore} from '@/stores/projects'
|
||||||
import {useAttachmentStore} from '@/stores/attachments'
|
import {useAttachmentStore} from '@/stores/attachments'
|
||||||
import {useKanbanStore} from '@/stores/kanban'
|
import {useKanbanStore} from '@/stores/kanban'
|
||||||
import {useBaseStore} from '@/stores/base'
|
import {useBaseStore} from '@/stores/base'
|
||||||
|
import ProjectUserService from '@/services/projectUsers'
|
||||||
|
|
||||||
|
interface MatchedAssignee extends IUser {
|
||||||
konrad marked this conversation as resolved
Outdated
dpschen
commented
This should be singular, like This should be singular, like `IUser`
konrad
commented
Done Done
|
|||||||
|
match: string,
|
||||||
|
}
|
||||||
|
|
||||||
// IDEA: maybe use a small fuzzy search here to prevent errors
|
// IDEA: maybe use a small fuzzy search here to prevent errors
|
||||||
dpschen marked this conversation as resolved
dpschen
commented
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
konrad
commented
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
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
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
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
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.
|
|||||||
function findPropertyByValue(object, key, value) {
|
function findPropertyByValue(object, key, value, fuzzy = false) {
|
||||||
return Object.values(object).find(
|
return Object.values(object).find(l => {
|
||||||
(l) => l[key]?.toLowerCase() === value.toLowerCase(),
|
if (fuzzy) {
|
||||||
)
|
return l[key]?.toLowerCase().includes(value.toLowerCase())
|
||||||
|
}
|
||||||
|
|
||||||
|
return l[key]?.toLowerCase() === value.toLowerCase()
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if the user exists in the search results
|
// Check if the user exists in the search results
|
||||||
|
@ -42,9 +50,19 @@ function validateUser(
|
||||||
users: IUser[],
|
users: IUser[],
|
||||||
query: IUser['username'] | IUser['name'] | IUser['email'],
|
query: IUser['username'] | IUser['name'] | IUser['email'],
|
||||||
) {
|
) {
|
||||||
return findPropertyByValue(users, 'username', query) ||
|
if (users.length === 1) {
|
||||||
|
return (
|
||||||
|
findPropertyByValue(users, 'username', query, true) ||
|
||||||
|
findPropertyByValue(users, 'name', query, true) ||
|
||||||
|
findPropertyByValue(users, 'email', query, true)
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
return (
|
||||||
konrad marked this conversation as resolved
Outdated
dpschen
commented
Fuzzy should be an option of
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
Done Done
|
|||||||
|
findPropertyByValue(users, 'username', query) ||
|
||||||
findPropertyByValue(users, 'name', query) ||
|
findPropertyByValue(users, 'name', query) ||
|
||||||
findPropertyByValue(users, 'email', query)
|
findPropertyByValue(users, 'email', query)
|
||||||
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if the label exists
|
// Check if the label exists
|
||||||
|
@ -63,14 +81,18 @@ async function addLabelToTask(task: ITask, label: ILabel) {
|
||||||
return response
|
return response
|
||||||
}
|
}
|
||||||
|
|
||||||
async function findAssignees(parsedTaskAssignees: string[]): Promise<IUser[]> {
|
async function findAssignees(parsedTaskAssignees: string[], projectId: number): Promise<MatchedAssignee[]> {
|
||||||
if (parsedTaskAssignees.length <= 0) {
|
if (parsedTaskAssignees.length <= 0) {
|
||||||
return []
|
return []
|
||||||
}
|
}
|
||||||
|
|
||||||
const userService = new UserService()
|
const userService = new ProjectUserService()
|
||||||
dpschen
commented
What I meant: Seems better to use ~~The bracket around seems to be unnecessary.~~
What I meant: Seems better to use `await` with `then` here
konrad
commented
Don't you mean Don't you mean `then` without `await`?
dpschen
commented
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
Then what's the advantage of the extra Then what's the advantage of the extra `then` over the current solution?
|
|||||||
const assignees = parsedTaskAssignees.map(async a => {
|
const assignees = parsedTaskAssignees.map(async a => {
|
||||||
const users = await userService.getAll({}, {s: a})
|
const users = (await userService.getAll({projectId}, {s: a}))
|
||||||
|
.map(u => ({
|
||||||
|
...u,
|
||||||
|
match: a,
|
||||||
|
}))
|
||||||
return validateUser(users, a)
|
return validateUser(users, a)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
@ -388,15 +410,15 @@ export const useTaskStore = defineStore('task', () => {
|
||||||
cancel()
|
cancel()
|
||||||
throw new Error('NO_PROJECT')
|
throw new Error('NO_PROJECT')
|
||||||
}
|
}
|
||||||
dpschen
commented
`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
That should not be a problem, because 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).
|
|||||||
|
|
||||||
const assignees = await findAssignees(parsedTask.assignees)
|
const assignees = await findAssignees(parsedTask.assignees, foundProjectId)
|
||||||
|
|
||||||
// Only clean up those assignees from the task title which actually exist
|
// Only clean up those assignees from the task title which actually exist
|
||||||
let cleanedTitle = parsedTask.text
|
let cleanedTitle = parsedTask.text
|
||||||
if (assignees.length > 0) {
|
if (assignees.length > 0) {
|
||||||
const assigneePrefix = PREFIXES[quickAddMagicMode]?.assignee
|
const assigneePrefix = PREFIXES[quickAddMagicMode]?.assignee
|
||||||
if (assigneePrefix) {
|
if (assigneePrefix) {
|
||||||
cleanedTitle = cleanupItemText(cleanedTitle, assignees.map(a => a.username), assigneePrefix)
|
cleanedTitle = cleanupItemText(cleanedTitle, assignees.map(a => a.match), assigneePrefix)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
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:
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 :)