Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assign users to tasks #2237

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

Assign users to tasks #2237

wants to merge 2 commits into from

Conversation

Anuj-Gupta4
Copy link
Collaborator

@Anuj-Gupta4 Anuj-Gupta4 commented Feb 28, 2025

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@Anuj-Gupta4 Anuj-Gupta4 added the backend Related to backend code label Feb 28, 2025
@Anuj-Gupta4 Anuj-Gupta4 requested a review from Sujanadh February 28, 2025 12:00
@github-actions github-actions bot added enhancement New feature or request migration Contains a DB migration labels Feb 28, 2025
@Anuj-Gupta4 Anuj-Gupta4 marked this pull request as ready for review March 3, 2025 07:56
project_id INTEGER NOT NULL,
task_id INTEGER NOT NULL,
user_id INTEGER NOT NULL,
assigned_at TIMESTAMPTZ NOT NULL DEFAULT now()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a new table for this?

I thought we would just have an ASSIGN event in the task events table?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding new things to the database should always be a last resort

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we rely solely on the task_events table, we'd either have multiple event records per user (causing redundancy and data bloat) or a separate field for assignees that stores a list of user IDs resulting in slower querying and an empty column for non-assignment events.

Copy link
Member

@spwoodcock spwoodcock Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - we need to reference both the assigner (eg manager) and the assigned (mapper) in one event.

Instead, how about we add a field to the tasks table assigned_to, then also make a task events entry.

The task event will have the user id for the assigner (plus a comment containing the username of who it was assigned to), while the assigned_to field will have the user id of the assigned.

Do you think that could be good?

(the task unlocking logic may have to be tweaked though to remove the assigned_to value from the tasks table though)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are to assign multiple users to a task, the field we add to the tasks table would need to be a list of integers (since the table can only have one row for each task ID). We would need to append or extend to the assigned_to field, which would be a list of user IDs. This is not the best approach.

It would be better to store it in a new task assignments table for faster querying, simple insert and delete for assignments and referential integrity with foreign keys. The assignments would also be automatically deleted without any new query code if the user, project or task were to be deleted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree - if we need to assign multiple users then this is the way to go πŸ‘

Assigning multiple users doesn't really fit into our model though. A task can only be locked by one user. The credit for mapping a task will only go to one user. I thought we always discussed team mapping as an informal process, not recorded.

Anyway, if this was discussed in my absence and agreed on, I will defer to your judgement πŸ˜„

Copy link
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments πŸ‘

I'm still in two minds about this. Task assignment seems like something ephemeral to me - an event that occurs, but something that can be easily overwritten or chaged and not data that needs to be recorded in a separate table.

The requirement for multiple users to be assigned at once is a new one for me though - we haven't discussed this before - perhaps I missed it. With that in mind as the key criteria this could make sense (although the implication here is that perhaps we need to redesign how task locking and completion works).

So in summary, I kinda disagree with this approach, but don't want to hold up the process if this is needed πŸ˜… Go ahead and merge when you think this is ready!

-- Create task_assignments table only if it does not exist
DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'task_assignments') THEN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a simpler syntax in postgres for this: create table if not exists task_assignments

Checking the information schema is only needed when this is not possible, such as with enums

@@ -467,6 +475,9 @@ ADD CONSTRAINT xlsforms_title_key UNIQUE (title);
ALTER TABLE ONLY public.geometrylog
ADD CONSTRAINT geometrylog_pkey PRIMARY KEY (id);

ALTER TABLE ONLY public.task_assignments
ADD CONSTRAINT task_assignments_pkey PRIMARY KEY (task_id, user_id);

-- Indexing

CREATE INDEX idx_projects_outline ON public.projects USING gist (outline);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need an index for faster lookup. Choosing the column depends what you will query on most frequently.

@spwoodcock
Copy link
Member

spwoodcock commented Mar 4, 2025

Oh by the way, I remembered what my original idea here was from months ago, as part of the event driven redesign (for reference).

  1. Manager assigns task to user
  2. Two events occur:
    • A MAP event is added for the assigned user on the task (I.e. it looks like they locked the task. If they visit FMTM they see a task locked by them - we can easily add a filter 'my tasks' on the frontend).
    • Notification / email is sent to user about the assignment.

I think this approach is simpler and avoids changing too much, but I'm sure it has downsides I have missed too (or forgot we discussed)

@Sujanadh
Copy link
Collaborator

Sujanadh commented Mar 4, 2025

Will there be a scenario where multiple users will be assigned to a single task? To whom do we assign tasks, mappers? Are assigning tasks and locking tasks the same? I think I am a bit out of context here. @spwoodcock @Anuj-Gupta4

@spwoodcock
Copy link
Member

Will there be a scenario where multiple users will be assigned to a single task? To whom do we assign tasks, mappers? Are assigning tasks and locking tasks the same? I think I am a bit out of context here. @spwoodcock @Anuj-Gupta4

  • I'm not sure if assigning a task to multiple people was a requirement from Tokha?

  • Tasks are assigned to mappers πŸ‘

  • We made a new event ASSIGN to handle assignment in another PR. So I guess the idea was to have an assign event with the user id of the manager, then a MAP (lock) event with the user id of the mapper

@spwoodcock
Copy link
Member

Screenshot_20250304-100743.png

Perhaps I made a mistake here when suggesting the ASSIGN event should automatically set the task state to LOCKED_FOR_MAPPING? As the assign event is by the manager, but we want the task locking to be done by the mapper.

Should we possibly revert this and instead create two event entries as mentioned above?

@Sujanadh
Copy link
Collaborator

Sujanadh commented Mar 4, 2025

I am not sure there will be only one user/mapper assigned to a task. Some projects might need numbers of enumerators to do field surveys even for a single task (if the number of features within the task is huge), like a group of people assigned to a single task. In the case of Tokha, there are 4,5 people in a single task to do a survey.

So, the concept of assigning tasks to users is to restrict users to not to select task that is not assigned to them?

@spwoodcock
Copy link
Member

spwoodcock commented Mar 4, 2025

I am not sure there will be only one user/mapper assigned to a task. Some projects might need numbers of enumerators to do field surveys even for a single task (if the number of features within the task is huge), like a group of people assigned to a single task. In the case of Tokha, there are 4,5 people in a single task to do a survey.

So, the concept of assigning tasks to users is to restrict users to not to select task that is not assigned to them?

Yep basically! Task assignment is pretty arbitrary - it only serves two purposes:

  • Lock the task so others don't try to map there
  • Notify the user that their manager wants them to map the task

Showing a task as assigned to multiple people might complicate the frontend.

Could we consider assigning the task to only one user, but notifying multiple people?

Its not perfect, but this kind of requirement suggests we need mapper teams that can be assigned to, not multiple individuals (still a 1 task to 1 mapper team assignment). We could look into that in the future.

@Sujanadh
Copy link
Collaborator

Sujanadh commented Mar 4, 2025

yeah we could do that. what if it restricts users who needs to do mapping on same task?

@spwoodcock
Copy link
Member

yeah we could do that. what if it restricts users who needs to do mapping on same task?

I think we removed the restrictions in place previously - any user should be able to map regardless of who locked a task.

Task locking should only be an indication / helper and not restrictive

@Sujanadh
Copy link
Collaborator

Sujanadh commented Mar 4, 2025

Okay, so we are just assigning users to let them know that it is assigned for you, just to notify?

@Anuj-Gupta4
Copy link
Collaborator Author

How about we use the task event table to assign only one user and lock the task but keep the task assignments table to keep track of all the mappers that are expected to map the task and were notified about it? There could be a small section in the frontend to show that (maybe only for the project manager) if needed. I think it's better to not implement any restrictions on the tasks as well.

@spwoodcock
Copy link
Member

Yeah that could work πŸ‘

But what about if the task gets re-assigned to new people? How do we handle that?

@Anuj-Gupta4
Copy link
Collaborator Author

I would say let's implement bulk assign and bulk remove for a single task. In the frontend, there would be a list of users assigned to a task, and we would allow the project manager to select multi users and either add or remove them. There would be OSM message and email for both assign and remove as well. What do you think?

@spwoodcock
Copy link
Member

spwoodcock commented Mar 5, 2025

We discussed on our team call today - decided to implement teams to keep a 1:1 assignment for tasks.

  • I suggest we keep this simple and only allow 'mapper' teams.
  • Plus Sujan suggested we make them project specific, instead of organisation specific.
  • Also for now I don't think we should have team invites or joining by the mapper themselves. Teams can be assigned by the manager only to keep things simple. Perhaps we add this to the project user management part: (1) team creation (2) assigning users to a team

table structure:

CREATE TABLE IF NOT EXISTS project_teams (
    team_id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
    # team_name is optional - should we set a default here so it's more easy to display on the UI?
    team_name VARCHAR,
    project_id INTEGER NOT NULL,
    user_id INTEGER NOT NULL
);

ALTER TABLE public.project_teams OWNER TO fmtm;

ALTER TABLE ONLY public.project_teams
  ADD CONSTRAINT project_teams_pkey PRIMARY KEY (team_id);

ALTER TABLE ONLY public.project_teams
  ADD CONSTRAINT fk_projects FOREIGN KEY (project_id) REFERENCES public.projects (id);

ALTER TABLE ONLY public.project_teams
  ADD CONSTRAINT fk_users FOREIGN KEY (user_id) REFERENCES public.users (id);

# index by project as it's the most important for searching
CREATE INDEX idx_project_teams ON public.project_teams USING btree (project_id);

Note

This would also mean we need to modify task_events to accept a team_id instead of a user_id.
The user_id field is nullable, so we should add a new field team_id integer (also nullable) and foreign key to project_teams.team_id field.
If the task is assigned, it can be assigned to EITHER an individual user, or a team.
Notifications are sent to all users in the team.

Any thoughts on if we need extra fields?

@Anuj-Gupta4
Copy link
Collaborator Author

Anuj-Gupta4 commented Mar 5, 2025

Won't this structure result in each team having only one user? Also, this table doesn't seem to be connected to tasks at all?

Edit: never mind, just read the note

@spwoodcock
Copy link
Member

You are right - we actually need two tables πŸ‘
It doesn't need to be linked to tasks - that's done in the task_events table πŸ˜„

@spwoodcock
Copy link
Member

spwoodcock commented Mar 5, 2025

Second try:

CREATE TABLE IF NOT EXISTS project_teams (
    team_id UUID DEFAULT gen_random_uuid(),
    # team_name is optional - should we set a default here so it's more easy to display on the UI?
    team_name VARCHAR,
    project_id INTEGER NOT NULL
);
ALTER TABLE public.project_teams OWNER TO fmtm;

ALTER TABLE ONLY public.project_teams
  ADD CONSTRAINT project_teams_pkey PRIMARY KEY (team_id);

ALTER TABLE ONLY public.project_teams
  ADD CONSTRAINT fk_projects FOREIGN KEY (project_id) REFERENCES public.projects (id);



CREATE TABLE IF NOT EXISTS project_team_users (
    team_id UUID PRIMARY KEY DEFAULT,
    user_id INTEGER NOT NULL
);
ALTER TABLE public.project_team_users OWNER TO fmtm;

# composite
ALTER TABLE ONLY public.project_team_users 
  ADD CONSTRAINT project_teams_pkey PRIMARY KEY (team_id, user_id );

ALTER TABLE ONLY public.project_team_users
  ADD CONSTRAINT fk_teams FOREIGN KEY (team_id) REFERENCES public.users (project_teams);

ALTER TABLE ONLY public.project_team_users
  ADD CONSTRAINT fk_users FOREIGN KEY (user_id) REFERENCES public.users (id);


# index users by their team id, to allow for faster lookup when joining these tables
CREATE INDEX idx_project_team_users_team_id 
  ON public.project_team_users (team_id);

Does this work?

@Anuj-Gupta4
Copy link
Collaborator Author

I think this should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code enhancement New feature or request migration Contains a DB migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants