From 3e90e848182c1d70be9b8c72d55a0d81e7ce65e8 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Sun, 3 Jul 2022 16:37:55 +0545 Subject: [PATCH 01/10] Add field for team join notification settings --- backend/config.py | 1 + backend/models/dtos/team_dto.py | 3 + backend/models/dtos/user_dto.py | 4 +- backend/models/postgis/team.py | 5 +- backend/models/postgis/user.py | 8 ++- backend/services/messaging/message_service.py | 7 ++- backend/services/team_service.py | 10 +-- migrations/versions/bcb474128817_.py | 61 +++++++++++++++++++ tests/backend/helpers/test_helpers.py | 6 +- .../services/messaging/test_smtp_service.py | 6 ++ .../integration/services/test_team_service.py | 61 ++++++++++++++++++- .../messaging/test_messaging_service.py | 7 ++- .../unit/services/test_mapping_service.py | 6 +- 13 files changed, 165 insertions(+), 20 deletions(-) create mode 100644 migrations/versions/bcb474128817_.py diff --git a/backend/config.py b/backend/config.py index bcb1f33346..523b49abb7 100644 --- a/backend/config.py +++ b/backend/config.py @@ -152,3 +152,4 @@ class TestEnvironmentConfig(EnvironmentConfig): + f"{POSTGRES_PORT}" + f"/test_{POSTGRES_DB}" ) + LOG_LEVEL = "DEBUG" diff --git a/backend/models/dtos/team_dto.py b/backend/models/dtos/team_dto.py index 634baf442f..365b89c9de 100644 --- a/backend/models/dtos/team_dto.py +++ b/backend/models/dtos/team_dto.py @@ -41,6 +41,9 @@ class TeamMembersDTO(Model): username = StringType(required=True) function = StringType(required=True, validators=[validate_team_member_function]) active = StringType() + join_request_notifications = BooleanType( + default=False, serialized_name="joinRequestNotifications" + ) picture_url = StringType(serialized_name="pictureUrl") diff --git a/backend/models/dtos/user_dto.py b/backend/models/dtos/user_dto.py index 6ce72e8260..f0a6ff6db9 100644 --- a/backend/models/dtos/user_dto.py +++ b/backend/models/dtos/user_dto.py @@ -79,7 +79,9 @@ class UserDTO(Model): tasks_comments_notifications = BooleanType( serialized_name="taskCommentsNotifications" ) - teams_notifications = BooleanType(serialized_name="teamsNotifications") + teams_announcement_notifications = BooleanType( + serialized_name="teamsAnnouncementNotifications" + ) # these are read only gender = StringType( diff --git a/backend/models/postgis/team.py b/backend/models/postgis/team.py index 6d18e1fbdc..2eb2ca9fe1 100644 --- a/backend/models/postgis/team.py +++ b/backend/models/postgis/team.py @@ -26,7 +26,9 @@ class TeamMembers(db.Model): ) function = db.Column(db.Integer, nullable=False) # either 'editor' or 'manager' active = db.Column(db.Boolean, default=False) - + join_request_notifications = db.Column( + db.Boolean, nullable=False, default=False + ) # Managers can turn notifications on/off for team join requests member = db.relationship( User, backref=db.backref("teams", cascade="all, delete-orphan") ) @@ -196,6 +198,7 @@ def as_dto_team_member(self, member) -> TeamMembersDTO: member_dto.function = member_function member_dto.picture_url = user.picture_url member_dto.active = member.active + member_dto.join_request_notifications = member.join_request_notifications return member_dto def as_dto_team_project(self, project) -> TeamProjectDTO: diff --git a/backend/models/postgis/user.py b/backend/models/postgis/user.py index c10a431df5..c28625aa31 100644 --- a/backend/models/postgis/user.py +++ b/backend/models/postgis/user.py @@ -61,7 +61,9 @@ class User(db.Model): projects_notifications = db.Column(db.Boolean, default=True, nullable=False) tasks_notifications = db.Column(db.Boolean, default=True, nullable=False) tasks_comments_notifications = db.Column(db.Boolean, default=False, nullable=False) - teams_notifications = db.Column(db.Boolean, default=True, nullable=False) + teams_announcement_notifications = db.Column( + db.Boolean, default=True, nullable=False + ) date_registered = db.Column(db.DateTime, default=timestamp) # Represents the date the user last had one of their tasks validated last_validation_date = db.Column(db.DateTime, default=timestamp) @@ -364,7 +366,9 @@ def as_dto(self, logged_in_username: str) -> UserDTO: user_dto.projects_comments_notifications = self.projects_comments_notifications user_dto.tasks_notifications = self.tasks_notifications user_dto.tasks_comments_notifications = self.tasks_comments_notifications - user_dto.teams_notifications = self.teams_notifications + user_dto.teams_announcement_notifications = ( + self.teams_announcement_notifications + ) if self.username == logged_in_username: # Only return email address and gender information when logged in user is looking at their own profile diff --git a/backend/services/messaging/message_service.py b/backend/services/messaging/message_service.py index 6b7ff6190c..55020c3043 100644 --- a/backend/services/messaging/message_service.py +++ b/backend/services/messaging/message_service.py @@ -169,7 +169,7 @@ def _push_messages(messages): ): continue if ( - user.teams_notifications is False + user.teams_announcement_notifications is False and obj.message_type == MessageType.TEAM_BROADCAST.value ): messages_objs.append(obj) @@ -321,8 +321,9 @@ def send_request_to_join_team( MessageService.get_user_link(from_username), MessageService.get_team_link(team_name, team_id, True), ) - message.add_message() - message.save() + MessageService._push_messages( + [dict(message=message, user=User.query.get(to_user))] + ) @staticmethod def accept_reject_request_to_join_team( diff --git a/backend/services/team_service.py b/backend/services/team_service.py index 671e83baa0..165b9a87cc 100644 --- a/backend/services/team_service.py +++ b/backend/services/team_service.py @@ -81,13 +81,13 @@ def join_team(team_id: int, requesting_user: int, username: str, role: str = Non active = True TeamService.add_team_member(team_id, user.id, role, active) - if team.invite_only: team_managers = team.get_team_managers() - for member in team_managers: - MessageService.send_request_to_join_team( - user.id, user.username, member.user_id, team.name, team_id - ) + for manager in team_managers: + if manager.join_request_notifications: + MessageService.send_request_to_join_team( + user.id, user.username, manager.user_id, team.name, team_id + ) @staticmethod def send_invite(team_id, from_user_id, username): diff --git a/migrations/versions/bcb474128817_.py b/migrations/versions/bcb474128817_.py new file mode 100644 index 0000000000..815b1e5c29 --- /dev/null +++ b/migrations/versions/bcb474128817_.py @@ -0,0 +1,61 @@ +"""empty message + +Revision ID: bcb474128817 +Revises: 8b61ac59bc57 +Create Date: 2022-07-03 10:46:56.075805 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = "bcb474128817" +down_revision = "8b61ac59bc57" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_index( + "idx_task_validation_validator_status_composite", + "task_invalidation_history", + ["invalidator_id", "is_closed"], + unique=False, + ) + op.add_column( + "team_members", + sa.Column("join_request_notifications", sa.Boolean(), nullable=True), + ) + op.add_column( + "users", + sa.Column("teams_announcement_notifications", sa.Boolean(), nullable=True), + ) + op.execute("UPDATE team_members SET join_request_notifications = false") + op.execute("UPDATE users SET teams_announcement_notifications = false") + op.alter_column("team_members", "join_request_notifications", nullable=False) + op.alter_column("users", "teams_announcement_notifications", nullable=False) + op.drop_column("users", "teams_notifications") + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column( + "users", + sa.Column( + "teams_notifications", + sa.BOOLEAN(), + server_default=sa.text("true"), + autoincrement=False, + nullable=False, + ), + ) + op.drop_column("users", "teams_announcement_notifications") + op.drop_column("team_members", "join_request_notifications") + op.drop_index( + "idx_task_validation_validator_status_composite", + table_name="task_invalidation_history", + ) + # ### end Alembic commands ### diff --git a/tests/backend/helpers/test_helpers.py b/tests/backend/helpers/test_helpers.py index 441c711473..5a4ff378ef 100644 --- a/tests/backend/helpers/test_helpers.py +++ b/tests/backend/helpers/test_helpers.py @@ -93,11 +93,11 @@ def get_canned_simplified_osm_user_details(): return data -def return_canned_user() -> User: +def return_canned_user(username=TEST_USERNAME, id=TEST_USER_ID) -> User: """Returns a canned user""" test_user = User() - test_user.username = TEST_USERNAME - test_user.id = TEST_USER_ID + test_user.username = username + test_user.id = id test_user.mapping_level = MappingLevel.BEGINNER.value test_user.email_address = None diff --git a/tests/backend/integration/services/messaging/test_smtp_service.py b/tests/backend/integration/services/messaging/test_smtp_service.py index b74d6723e9..94ef38ccee 100644 --- a/tests/backend/integration/services/messaging/test_smtp_service.py +++ b/tests/backend/integration/services/messaging/test_smtp_service.py @@ -17,6 +17,7 @@ def setUp(self): self.from_username = "Aadesh Baral" self.message_id = 1 self.project_id = 1 + self.project_name = "Test Project" self.task_id = 1 self.subject = "test subject" self.content = "test content" @@ -43,6 +44,7 @@ def test_send_alert(self): message_id=self.message_id, from_username=self.from_username, project_id=self.project_id, + project_name=self.project_name, task_id=self.task_id, subject=self.subject, content=self.content, @@ -63,6 +65,7 @@ def test_send_alert_message_limits(self): message_id=self.message_id, from_username=self.from_username, project_id=self.project_id, + project_name=self.project_name, task_id=self.task_id, subject=self.subject, content=self.content, @@ -81,6 +84,7 @@ def test_alert_not_sent_if_email_not_supplied(self): message_id=self.message_id, from_username=self.from_username, project_id=self.project_id, + project_name=self.project_name, task_id=self.task_id, subject=self.subject, content=self.content, @@ -96,6 +100,7 @@ def test_does_not_send_if_user_not_verified(self): message_id=self.message_id, from_username=self.from_username, project_id=self.project_id, + project_name=self.project_name, task_id=self.task_id, subject=self.subject, content=self.content, @@ -115,6 +120,7 @@ def test_does_send_if_user_verified(self, mock_send_message): message_id=self.message_id, from_username=self.from_username, project_id=self.project_id, + project_name=self.project_name, task_id=self.task_id, subject=self.subject, content=self.content, diff --git a/tests/backend/integration/services/test_team_service.py b/tests/backend/integration/services/test_team_service.py index 0095be78c5..41d4d5e687 100644 --- a/tests/backend/integration/services/test_team_service.py +++ b/tests/backend/integration/services/test_team_service.py @@ -1,11 +1,19 @@ +from unittest.mock import patch + from backend.models.postgis.statuses import TeamMemberFunctions, TeamRoles -from backend.services.team_service import TeamService +from backend.services.team_service import ( + TeamService, + TeamJoinNotAllowed, + MessageService + ) from tests.backend.base import BaseTestCase from tests.backend.helpers.test_helpers import ( add_user_to_team, assign_team_to_project, create_canned_project, create_canned_team, + create_canned_user, + return_canned_user ) @@ -86,3 +94,54 @@ def test_get_project_teams_as_dto(self): # Assert self.assertIn("teams", teams_dto) self.assertNotEqual(len(teams_dto["teams"]), 0) + + @patch.object(TeamService, "is_user_team_member") + def test_join_team_raises_error_if_user_already_team_member( + self, mock_is_team_member + ): + # Arrange + test_team = create_canned_team() + test_user = create_canned_user() + mock_is_team_member.return_value = True + # Act/Assert + with self.assertRaises(TeamJoinNotAllowed): + TeamService.join_team( + test_team.id, test_user.id, test_user.username, "MEMBER" + ) + + @patch.object(TeamService, "is_user_team_member") + @patch.object(MessageService, "_push_messages") + def test_join_team_sends_notification_if_team_is_invite_only_and_manager_has_allowed_notification( + self, mock_send_notification, mock_is_team_member + ): + # Arrange + test_team = create_canned_team() + test_team.invite_only = True + test_user = create_canned_user() + mock_is_team_member.return_value = False + test_manager = return_canned_user("test manager", 1234) + test_manager = add_user_to_team(test_team, test_manager, 1, True) + test_manager.join_request_notifications = True + + # Act + TeamService.join_team(test_team.id, test_user.id, test_user.username, "MEMBER") + # Assert + mock_send_notification.assert_called() + + @patch.object(TeamService, "is_user_team_member") + @patch.object(MessageService, "_push_messages") + def test_join_team_doesnt_send_notification_if_manager_has_disallowed_notification( + self, mock_send_notification, mock_is_team_member + ): + # Arrange + test_team = create_canned_team() + test_team.invite_only = True + test_user = create_canned_user() + mock_is_team_member.return_value = False + test_manager = return_canned_user("test manager", 1234) + test_manager = add_user_to_team(test_team, test_manager, 1, True) + test_manager.join_request_notifications = False + # Act + TeamService.join_team(test_team.id, test_user.id, test_user.username, "MEMBER") + # Assert + mock_send_notification.assert_not_called() diff --git a/tests/backend/unit/services/messaging/test_messaging_service.py b/tests/backend/unit/services/messaging/test_messaging_service.py index 2bee527e8b..f392c15c76 100644 --- a/tests/backend/unit/services/messaging/test_messaging_service.py +++ b/tests/backend/unit/services/messaging/test_messaging_service.py @@ -26,19 +26,20 @@ def test_message_service_generates_correct_task_link(self): def test_message_service_generates_correct_chat_link(self): # Act link = MessageService.get_project_link( - 1, "http://test.com", include_chat_section=True + 1, "Test Project", "http://test.com", include_chat_section=True ) self.assertEqual( link, - 'Project 1', + 'Test Project', ) link = MessageService.get_project_link( 1, + "Test Project", "http://test.com", ) self.assertEqual( link, - 'Project 1', + 'Test Project', ) diff --git a/tests/backend/unit/services/test_mapping_service.py b/tests/backend/unit/services/test_mapping_service.py index 0ddff7a478..5064bcfa26 100644 --- a/tests/backend/unit/services/test_mapping_service.py +++ b/tests/backend/unit/services/test_mapping_service.py @@ -1,3 +1,4 @@ +from unittest.mock import patch, MagicMock from backend.services.mapping_service import ( MappingService, Task, @@ -11,7 +12,7 @@ ) from backend.models.dtos.mapping_dto import MappedTaskDTO, LockTaskDTO from backend.models.postgis.task import TaskHistory, TaskAction, User -from unittest.mock import patch, MagicMock +from backend.services.messaging.message_service import MessageService from tests.backend.base import BaseTestCase @@ -127,10 +128,12 @@ def test_if_new_state_not_acceptable_raise_error(self, mock_task): @patch.object(Task, "update") @patch.object(TaskHistory, "get_last_status") @patch.object(TaskHistory, "update_task_locked_with_duration") + @patch.object(MessageService, "send_message_after_comment") @patch.object(MappingService, "get_task") def test_unlock_with_comment_sets_history( self, mock_task, + mock_send_message, mock_history, mock_update, mock_stats, @@ -147,6 +150,7 @@ def test_unlock_with_comment_sets_history( test_task = MappingService.unlock_task_after_mapping(self.mapped_task_dto) # Assert + mock_send_message.assert_called() self.assertEqual(TaskAction.COMMENT.name, test_task.task_history[0].action) self.assertEqual(test_task.task_history[0].action_text, "Test comment") From 979884279f0d9ae0fae5f270aaf2af18bd30d6ad Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Sun, 3 Jul 2022 16:40:09 +0545 Subject: [PATCH 02/10] Modify teamJoinRequests switch --- frontend/src/components/user/forms/notifications.js | 2 +- tests/backend/integration/services/test_team_service.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/user/forms/notifications.js b/frontend/src/components/user/forms/notifications.js index 641d493569..318cf11d48 100644 --- a/frontend/src/components/user/forms/notifications.js +++ b/frontend/src/components/user/forms/notifications.js @@ -16,7 +16,7 @@ export function UserNotificationsForm(props) { - + diff --git a/tests/backend/integration/services/test_team_service.py b/tests/backend/integration/services/test_team_service.py index 41d4d5e687..351aeef3a4 100644 --- a/tests/backend/integration/services/test_team_service.py +++ b/tests/backend/integration/services/test_team_service.py @@ -4,8 +4,8 @@ from backend.services.team_service import ( TeamService, TeamJoinNotAllowed, - MessageService - ) + MessageService, +) from tests.backend.base import BaseTestCase from tests.backend.helpers.test_helpers import ( add_user_to_team, @@ -13,7 +13,7 @@ create_canned_project, create_canned_team, create_canned_user, - return_canned_user + return_canned_user, ) From 352465152dd4421609b72f7adb24e643d8a02258 Mon Sep 17 00:00:00 2001 From: Hel Nershing Thapa Date: Tue, 5 Jul 2022 14:03:01 +0545 Subject: [PATCH 03/10] Add switch component --- .../src/components/teamsAndOrgs/members.js | 41 ++++++++++++++++++- .../src/components/teamsAndOrgs/messages.js | 5 +++ frontend/src/views/teams.js | 3 ++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/teamsAndOrgs/members.js b/frontend/src/components/teamsAndOrgs/members.js index e0bd266bbf..c2d276a904 100644 --- a/frontend/src/components/teamsAndOrgs/members.js +++ b/frontend/src/components/teamsAndOrgs/members.js @@ -8,6 +8,7 @@ import messages from './messages'; import { UserAvatar } from '../user/avatar'; import { EditModeControl } from './editMode'; import { Button } from '../button'; +import { SwitchToggle } from '../formInputs'; import { fetchLocalJSONAPI, pushToLocalJSONAPI } from '../../network/genericJSONRequest'; import { Alert } from '../alert'; import { useOnClickOutside } from '../../hooks/UseOnClickOutside'; @@ -152,8 +153,20 @@ export function Members({ ); } -export function JoinRequests({ requests, teamId, addMembers, updateRequests }: Object) { +export function JoinRequests({ + requests, + teamId, + addMembers, + updateRequests, + members, + updateTeam, + isTeamInviteOnly, +}: Object) { const token = useSelector((state) => state.auth.get('token')); + const loggedInUsername = useSelector((state) => state.auth.get('userDetails')).username; + const isJoinRequestEnabled = members?.filter((member) => member.username === loggedInUsername)[0] + ?.joinRequestNotifications; + const [isChecked, setIsChecked] = useState(isJoinRequestEnabled); const acceptRejectRequest = useCallback( (user, action) => { @@ -173,6 +186,18 @@ export function JoinRequests({ requests, teamId, addMembers, updateRequests }: O [teamId, requests, updateRequests, addMembers, token], ); + const handleJoinRequestNotificationsChange = (e) => { + const { checked } = e.target; + setIsChecked(checked); + let tempMembers = members; + const memberIndex = tempMembers.findIndex((member) => member.username === loggedInUsername); + Object.assign(tempMembers[memberIndex], { + joinRequestNotifications: checked, + active: checked.toString(), + }); + updateTeam({ members }); + }; + return (
@@ -180,6 +205,18 @@ export function JoinRequests({ requests, teamId, addMembers, updateRequests }: O
+ {isTeamInviteOnly && ( +
+ +
+ handleJoinRequestNotificationsChange(e)} + labelPosition="right" + /> +
+
+ )}
{requests.map((user, n) => (
@@ -210,7 +247,7 @@ export function JoinRequests({ requests, teamId, addMembers, updateRequests }: O
))} {requests.length === 0 && ( -
+
)} diff --git a/frontend/src/components/teamsAndOrgs/messages.js b/frontend/src/components/teamsAndOrgs/messages.js index 7fa351ed9e..d1036e2669 100644 --- a/frontend/src/components/teamsAndOrgs/messages.js +++ b/frontend/src/components/teamsAndOrgs/messages.js @@ -409,6 +409,11 @@ export default defineMessages({ id: 'management.teams.invite_only.description', defaultMessage: "Managers need to approve a member's request to join.", }, + newJoinRequestNotification: { + id: 'management.teams.newJoinRequestNotification', + defaultMessage: + 'Enable for team managers to receive (email) notifications each time a new join request is made', + }, waitingApproval: { id: 'teamsAndOrgs.management.teams.messages.waiting_approval', defaultMessage: 'Your request to join this team is waiting for approval.', diff --git a/frontend/src/views/teams.js b/frontend/src/views/teams.js index 19006c8a76..8b719aab2a 100644 --- a/frontend/src/views/teams.js +++ b/frontend/src/views/teams.js @@ -339,6 +339,9 @@ export function EditTeam(props) { teamId={team.teamId} addMembers={addMembers} updateRequests={setRequests} + members={members} + updateTeam={updateTeam} + isTeamInviteOnly={team.inviteOnly} />
From e38c0696c862417a7f9ed574ce46f2665e4a22f1 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Tue, 5 Jul 2022 17:00:41 +0545 Subject: [PATCH 04/10] Fix issue while updating team members --- backend/models/dtos/team_dto.py | 2 +- backend/models/postgis/team.py | 34 ++++++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/backend/models/dtos/team_dto.py b/backend/models/dtos/team_dto.py index 365b89c9de..9273464deb 100644 --- a/backend/models/dtos/team_dto.py +++ b/backend/models/dtos/team_dto.py @@ -142,7 +142,7 @@ class UpdateTeamDTO(Model): name = StringType() logo = StringType() description = StringType() - invite_only = BooleanType(default=False, serialized_name="inviteOnly") + invite_only = BooleanType(serialized_name="inviteOnly") visibility = StringType( validators=[validate_team_visibility], serialize_when_none=False ) diff --git a/backend/models/postgis/team.py b/backend/models/postgis/team.py index 2eb2ca9fe1..564c7c0074 100644 --- a/backend/models/postgis/team.py +++ b/backend/models/postgis/team.py @@ -46,6 +46,15 @@ def delete(self): db.session.delete(self) db.session.commit() + def update(self): + """ Updates the current model in the DB """ + db.session.commit() + + @staticmethod + def get(team_id: int, user_id: int): + """ Returns a team member by team_id and user_id """ + return TeamMembers.query.filter_by(team_id=team_id, user_id=user_id).first() + class Team(db.Model): """ Describes a team """ @@ -124,18 +133,25 @@ def update(self, team_dto: TeamDTO): if team_dto.members != self._get_team_members() and team_dto.members: for member in self.members: - db.session.delete(member) - + member_name = User.get_by_id(member.user_id).username + if member_name not in [i["username"] for i in team_dto.members]: + member.delete() for member in team_dto.members: - user = User.get_by_username(member["userName"]) - + user = User.get_by_username(member["username"]) if user is None: raise NotFound("User not found") - - new_team_member = TeamMembers() - new_team_member.team = self - new_team_member.member = user - new_team_member.function = TeamMemberFunctions[member["function"]].value + team_member = TeamMembers.get(self.id, user.id) + if team_member: + team_member.join_request_notifications = member[ + "join_request_notifications" + ] + else: + new_team_member = TeamMembers() + new_team_member.team = self + new_team_member.member = user + new_team_member.function = TeamMemberFunctions[ + member["function"] + ].value db.session.commit() From b4c6462ee891d037c3144097f4264d824dbebde9 Mon Sep 17 00:00:00 2001 From: Hel Nershing Thapa Date: Wed, 6 Jul 2022 14:44:14 +0545 Subject: [PATCH 05/10] responsive rerendering on team details update --- .../src/components/teamsAndOrgs/members.js | 31 +++++++++++++------ .../teamsAndOrgs/tests/members.test.js | 6 ++-- frontend/src/views/teams.js | 9 ++++-- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/frontend/src/components/teamsAndOrgs/members.js b/frontend/src/components/teamsAndOrgs/members.js index c2d276a904..1e77f68de2 100644 --- a/frontend/src/components/teamsAndOrgs/members.js +++ b/frontend/src/components/teamsAndOrgs/members.js @@ -158,15 +158,26 @@ export function JoinRequests({ teamId, addMembers, updateRequests, - members, + managers, updateTeam, isTeamInviteOnly, }: Object) { const token = useSelector((state) => state.auth.get('token')); - const loggedInUsername = useSelector((state) => state.auth.get('userDetails')).username; - const isJoinRequestEnabled = members?.filter((member) => member.username === loggedInUsername)[0] - ?.joinRequestNotifications; - const [isChecked, setIsChecked] = useState(isJoinRequestEnabled); + const { username: loggedInUsername } = useSelector((state) => state.auth.get('userDetails')); + + const showJoinRequestSwitch = + isTeamInviteOnly && + managers?.filter( + (manager) => manager.username === loggedInUsername && manager.function === 'MANAGER', + ).length > 0; + const [isChecked, setIsChecked] = useState(false); + + useEffect(() => { + const isJoinRequestEnabled = managers.filter( + (manager) => manager.username === loggedInUsername, + )[0]?.joinRequestNotifications; + setIsChecked(isJoinRequestEnabled); + }, [loggedInUsername, managers]); const acceptRejectRequest = useCallback( (user, action) => { @@ -189,13 +200,13 @@ export function JoinRequests({ const handleJoinRequestNotificationsChange = (e) => { const { checked } = e.target; setIsChecked(checked); - let tempMembers = members; - const memberIndex = tempMembers.findIndex((member) => member.username === loggedInUsername); - Object.assign(tempMembers[memberIndex], { + let member = managers.find((member) => member.username === loggedInUsername); + + Object.assign(member, { joinRequestNotifications: checked, active: checked.toString(), }); - updateTeam({ members }); + updateTeam({ members: [member] }); }; return ( @@ -205,7 +216,7 @@ export function JoinRequests({
- {isTeamInviteOnly && ( + {showJoinRequestSwitch && (
diff --git a/frontend/src/components/teamsAndOrgs/tests/members.test.js b/frontend/src/components/teamsAndOrgs/tests/members.test.js index 30a5ab5594..7779c01129 100644 --- a/frontend/src/components/teamsAndOrgs/tests/members.test.js +++ b/frontend/src/components/teamsAndOrgs/tests/members.test.js @@ -19,7 +19,7 @@ describe('test JoinRequest list', () => { ]; const element = createComponentWithIntl( - + , ); const testInstance = element.root; @@ -53,7 +53,7 @@ describe('test JoinRequest list', () => { describe('test JoinRequest list without requests', () => { const element = createComponentWithIntl( - + , ); const testInstance = element.root; @@ -92,7 +92,7 @@ describe('test JoinRequest list without requests', () => { ); }); it('no requests message is present', () => { - expect(testInstance.findByProps({ className: 'tc' }).children[0].props.id).toBe( + expect(testInstance.findByProps({ className: 'tc mt3' }).children[0].props.id).toBe( 'management.teams.join_requests.empty', ); }); diff --git a/frontend/src/views/teams.js b/frontend/src/views/teams.js index 8b719aab2a..8e814a039d 100644 --- a/frontend/src/views/teams.js +++ b/frontend/src/views/teams.js @@ -8,6 +8,7 @@ import messages from './messages'; import { useFetch } from '../hooks/UseFetch'; import { useEditTeamAllowed } from '../hooks/UsePermissions'; import { useSetTitleTag } from '../hooks/UseMetaTags'; +import useForceUpdate from '../hooks/UseForceUpdate'; import { fetchLocalJSONAPI, pushToLocalJSONAPI } from '../network/genericJSONRequest'; import { getMembersDiff, @@ -210,7 +211,8 @@ export function CreateTeam() { export function EditTeam(props) { const userDetails = useSelector((state) => state.auth.get('userDetails')); const token = useSelector((state) => state.auth.get('token')); - const [error, loading, team] = useFetch(`teams/${props.id}/`); + const [forceUpdated, forceUpdate] = useForceUpdate(); + const [error, loading, team] = useFetch(`teams/${props.id}/`, forceUpdated); const [initManagers, setInitManagers] = useState(false); const [managers, setManagers] = useState([]); const [members, setMembers] = useState([]); @@ -218,7 +220,7 @@ export function EditTeam(props) { const [canUserEditTeam] = useEditTeamAllowed(team); const [memberJoinTeamError, setMemberJoinTeamError] = useState(null); const [managerJoinTeamError, setManagerJoinTeamError] = useState(null); - + useEffect(() => { if (!initManagers && team && team.members) { setManagers(filterActiveManagers(team.members)); @@ -276,6 +278,7 @@ export function EditTeam(props) { const updateTeam = (payload) => { pushToLocalJSONAPI(`teams/${props.id}/`, JSON.stringify(payload), token, 'PATCH'); + forceUpdate(); }; if (team && team.teamId && !canUserEditTeam) { @@ -339,7 +342,7 @@ export function EditTeam(props) { teamId={team.teamId} addMembers={addMembers} updateRequests={setRequests} - members={members} + managers={managers} updateTeam={updateTeam} isTeamInviteOnly={team.inviteOnly} /> From c1df22410b4ed962fa903f4d8731196d0ac07688 Mon Sep 17 00:00:00 2001 From: Aadesh Baral <67958673+Aadesh-Baral@users.noreply.github.com> Date: Sun, 24 Jul 2022 11:55:44 +0545 Subject: [PATCH 06/10] Feature/5247 transfer project ownership (#5250) * Fix transfer project ownership API * Replace text field with dropdown to select org manager * Add logic to enable/disable button * Send email to org managers after project ownership transfer * Add tests for transfer project ownership * Set isLoading status when managers are being fetched --- backend/api/projects/actions.py | 7 +- backend/services/messaging/message_service.py | 61 +++++++++++++ .../messaging/templates/been_some_time.html | 16 ++-- .../messaging/templates/profile-progress.html | 4 +- .../templates/project_transfer_alert_en.html | 20 +++++ .../templates/validator_role_unlocked.html | 4 +- .../services/messaging/templates/welcome.html | 2 +- backend/services/project_admin_service.py | 43 ++++++--- docs_old/error_code.md | 10 +-- .../src/components/projectEdit/actionsForm.js | 88 +++++++++---------- .../src/components/projectEdit/messages.js | 2 +- frontend/src/locales/en.json | 2 +- frontend/src/views/projectEdit.js | 3 +- tests/backend/helpers/test_helpers.py | 17 +++- .../messaging/test_messaging_service.py | 18 ++++ .../services/test_project_admin_service.py | 63 +++++++++++++ 16 files changed, 276 insertions(+), 84 deletions(-) create mode 100644 backend/services/messaging/templates/project_transfer_alert_en.html diff --git a/backend/api/projects/actions.py b/backend/api/projects/actions.py index 7c5f7abe15..f43152b22f 100644 --- a/backend/api/projects/actions.py +++ b/backend/api/projects/actions.py @@ -6,7 +6,10 @@ from backend.models.dtos.message_dto import MessageDTO from backend.models.dtos.grid_dto import GridDTO from backend.services.project_service import ProjectService, NotFound -from backend.services.project_admin_service import ProjectAdminService +from backend.services.project_admin_service import ( + ProjectAdminService, + ProjectAdminServiceError, +) from backend.services.grid.grid_service import GridService from backend.services.messaging.message_service import MessageService from backend.services.users.authentication_service import token_auth, tm @@ -63,7 +66,7 @@ def post(self, project_id): project_id, authenticated_user_id, username ) return {"Success": "Project Transferred"}, 200 - except ValueError as e: + except (ValueError, ProjectAdminServiceError) as e: return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 except Exception as e: error_msg = f"ProjectsActionsTransferAPI POST - unhandled error: {str(e)}" diff --git a/backend/services/messaging/message_service.py b/backend/services/messaging/message_service.py index 9b61cf495e..c11c103740 100644 --- a/backend/services/messaging/message_service.py +++ b/backend/services/messaging/message_service.py @@ -18,10 +18,12 @@ from backend.models.postgis.statuses import TeamRoles from backend.services.messaging.smtp_service import SMTPService from backend.services.messaging.template_service import ( + get_template, get_txt_template, template_var_replacing, clean_html, ) +from backend.services.organisation_service import OrganisationService from backend.services.users.user_service import UserService, User @@ -290,6 +292,55 @@ def send_message_after_comment( MessageService._push_messages(messages) + @staticmethod + def send_project_transfer_message( + project_id: int, + transferred_to: str, + transferred_by: str, + ): + """Will send a message to the manager of the organization after a project is transferred""" + app = ( + create_app() + ) # Because message-all run on background thread it needs it's own app context + + with app.app_context(): + project = Project.get(project_id) + project_name = project.get_project_title(project.default_locale) + + message = Message() + message.message_type = MessageType.PROJECT_ACTIVITY_NOTIFICATION.value + message.subject = ( + f"Project {project_name} was transferred to {transferred_to}" + ) + message.message = ( + f"Project {project_name} associated with your " + + f"organisation {project.organisation.name} was transferred to {transferred_to} by {transferred_by}." + ) + values = { + "PROJECT_ORG_NAME": project.organisation.name, + "PROJECT_ORG_ID": project.organisation_id, + "PROJECT_NAME": project_name, + "PROJECT_ID": project_id, + "TRANSFERRED_TO": transferred_to, + "TRANSFERRED_BY": transferred_by, + } + html_template = get_template("project_transfer_alert_en.html", values) + + managers = OrganisationService.get_organisation_by_id_as_dto( + project.organisation_id, User.get_by_username(transferred_by).id, False + ).managers + for manager in managers: + manager = UserService.get_user_by_username(manager.username) + message.to_user_id = manager.id + message.save() + if manager.email_address and manager.is_email_verified: + SMTPService._send_message( + manager.email_address, + message.subject, + html_template, + message.message, + ) + @staticmethod def get_user_link(username: str): base_url = current_app.config["APP_BASE_URL"] @@ -733,3 +784,13 @@ def get_user_settings_link(section=None, base_url=None) -> str: base_url = current_app.config["APP_BASE_URL"] return f'User Settings' + + @staticmethod + def get_organisation_link( + organisation_id: int, organisation_name: str, base_url=None + ) -> str: + """Helper method to generate a link to a user profile""" + if not base_url: + base_url = current_app.config["APP_BASE_URL"] + + return f'{organisation_name}' diff --git a/backend/services/messaging/templates/been_some_time.html b/backend/services/messaging/templates/been_some_time.html index abfb54b954..0d36c28427 100644 --- a/backend/services/messaging/templates/been_some_time.html +++ b/backend/services/messaging/templates/been_some_time.html @@ -19,7 +19,7 @@

- +
@@ -40,6 +40,7 @@
- +
@@ -100,7 +100,7 @@
- +
diff --git a/backend/services/messaging/templates/project_transfer_alert_en.html b/backend/services/messaging/templates/project_transfer_alert_en.html new file mode 100644 index 0000000000..95b7b0551d --- /dev/null +++ b/backend/services/messaging/templates/project_transfer_alert_en.html @@ -0,0 +1,20 @@ +{% extends "base.html" %} +{% block content %} +
+
+

+ Project + {{values['PROJECT_NAME']}} + associated with your organisation + {{values['PROJECT_ORG_NAME']}} + has been transferred to + {{values['TRANSFERRED_TO']}} + by + {{values['TRANSFERRED_BY']}}. +

+

+ Please ignore this email if you have received it by mistake.
+ Many thanks,
+ {{values['ORG_NAME']}}
+
+{% endblock %} \ No newline at end of file diff --git a/backend/services/messaging/templates/validator_role_unlocked.html b/backend/services/messaging/templates/validator_role_unlocked.html index 2e087a6ba7..b8add859ac 100644 --- a/backend/services/messaging/templates/validator_role_unlocked.html +++ b/backend/services/messaging/templates/validator_role_unlocked.html @@ -32,7 +32,7 @@ mapping skills and it shows. You've accomplished a lot to get to this moment:

- +

+ diff --git a/backend/services/messaging/templates/welcome.html b/backend/services/messaging/templates/welcome.html index 0a4c4c19a8..6dacb4ba5e 100644 --- a/backend/services/messaging/templates/welcome.html +++ b/backend/services/messaging/templates/welcome.html @@ -32,7 +32,7 @@ Learning_steps -
+
diff --git a/backend/services/project_admin_service.py b/backend/services/project_admin_service.py index 497bd2be2e..7036f87ad0 100644 --- a/backend/services/project_admin_service.py +++ b/backend/services/project_admin_service.py @@ -1,5 +1,5 @@ import json - +import threading import geojson from flask import current_app @@ -12,9 +12,11 @@ from backend.models.postgis.project import Project, Task, ProjectStatus from backend.models.postgis.statuses import TaskCreationMode, TeamRoles from backend.models.postgis.task import TaskHistory, TaskStatus, TaskAction +from backend.models.postgis.user import User from backend.models.postgis.utils import NotFound, InvalidData, InvalidGeoJson from backend.services.grid.grid_service import GridService from backend.services.license_service import LicenseService +from backend.services.messaging.message_service import MessageService from backend.services.users.user_service import UserService from backend.services.organisation_service import OrganisationService from backend.services.team_service import TeamService @@ -282,23 +284,40 @@ def get_projects_for_admin( def transfer_project_to(project_id: int, transfering_user_id: int, username: str): """Transfers project from old owner (transfering_user_id) to new owner (username)""" project = Project.get(project_id) + new_owner = UserService.get_user_by_username(username) + # No operation is required if the new owner is same as old owner + if username == project.author.username: + return # Check permissions for the user (transferring_user_id) who initiatied the action - if not ProjectAdminService.is_user_action_permitted_on_project( - transfering_user_id, project_id - ): - raise ValueError("UserNotPermitted- User action not permitted") - new_owner = UserService.get_user_by_username(username) + is_admin = UserService.is_user_an_admin(transfering_user_id) + is_author = UserService.is_user_the_project_author( + transfering_user_id, project.author_id + ) + is_org_manager = OrganisationService.is_user_an_org_manager( + project.organisation_id, transfering_user_id + ) + if not is_admin and not is_author and not is_org_manager: + raise ProjectAdminServiceError( + "TransferPermissionError- User does not have permissions to transfer project" + ) - # Check permissions for the new owner - must be an admin or project's org manager or a PM team member - if not ProjectAdminService.is_user_action_permitted_on_project( - new_owner.id, project_id + # Check permissions for the new owner - must be project's org manager + if not OrganisationService.is_user_an_org_manager( + project.organisation_id, new_owner.id ): - raise ValueError( - "InvalidNewOwner- New owner must be an admin or project's org manager or a PM team member" - ) + error_message = "InvalidNewOwner- New owner must be project's org manager" + if current_app: + current_app.logger.debug(error_message) + raise ValueError(error_message) else: + transferred_by = User.get_by_id(transfering_user_id).username + project.author_id = new_owner.id project.save() + threading.Thread( + target=MessageService.send_project_transfer_message, + args=(project_id, username, transferred_by), + ).start() @staticmethod def is_user_action_permitted_on_project( diff --git a/docs_old/error_code.md b/docs_old/error_code.md index 825259f4bf..0ba15e279c 100644 --- a/docs_old/error_code.md +++ b/docs_old/error_code.md @@ -11,7 +11,7 @@ When the TM API returns error messages, it does so in JSON format. For example, ### Error Codes In addition to descriptive error text, error messages also contains SubCodes. While the **text for an error message may change, the SubCode will stay the same**. -| Code | Subcode | Text | +| Code | Subcode | Text | | ---- | ------------------------ | ---------------------------------------------------------------------------------- | | 403 | AlreadyFeatured | Project is already featured | | 403 | CannotValidateMappedTask | Tasks cannot be validated by the same user who marked task as mapped or badimagery | @@ -19,7 +19,7 @@ In addition to descriptive error text, error messages also contains SubCodes. Wh | 400 | InvalidData | Error validating request | | 400 | InvalidDateRange | Date range can not be bigger than 1 year | | 400 | InvalidMultipolygon | Area of Interest: Invalid MultiPolygon | -| 403 | InvalidNewOwner | New owner must be an admin or project's org manager or a PM team member | +| 403 | InvalidNewOwner | New owner must be project's org manager | | 400 | InvalidStartDate | Start date must be earlier than end date | | 403 | InvalidTaskState | Task in invalid state for mapping | | 403 | InvalidUnlockState | Can only set status to MAPPED, BADIMAGERY, READY after mapping | @@ -44,8 +44,7 @@ In addition to descriptive error text, error messages also contains SubCodes. Wh | 403 | UserNotAllowed | Mapping not allowed because: User not on allowed list | | 403 | UserNotPermitted | User action not permitted | | 403 | UserPermissionError | User is not a manager of the project | -| 403 | PrivateProject | User not permitted: Private Project -| +| 403 | PrivateProject | User not permitted: Private Project | | 403 | ProjectNotFetched | Unable to fetch project | | 403 | NotPermittedToCreate | User is not permitted to create project | | 400 | MustBeFeatureCollection | GeoJson must be FeatureCollection | @@ -85,4 +84,5 @@ In addition to descriptive error text, error messages also contains SubCodes. Wh | 403 | OrgHasProjects | Organization has some projects | | 403 | MustHaveAdmin | Must have at least one admin | | 403 | LoginToFilterManager | Filter by manager\_user\_id is not allowed to unauthenticated requests | -| 400 | SelfIntersectingAOI | Invalid geometry. Polygon is self-intersecting | \ No newline at end of file +| 400 | SelfIntersectingAOI | Invalid geometry. Polygon is self-intersecting | +| 400 | TransferPermissionError | Project ownership transfer is only allowed to TM Admin, Organization admin and project author| \ No newline at end of file diff --git a/frontend/src/components/projectEdit/actionsForm.js b/frontend/src/components/projectEdit/actionsForm.js index b5068cca76..889273bd47 100644 --- a/frontend/src/components/projectEdit/actionsForm.js +++ b/frontend/src/components/projectEdit/actionsForm.js @@ -1,6 +1,7 @@ -import React, { useState } from 'react'; +import React, { useState, useContext } from 'react'; import { useSelector } from 'react-redux'; import Popup from 'reactjs-popup'; +import Select from 'react-select'; import { navigate } from '@reach/router'; import { useDropzone } from 'react-dropzone'; import { FormattedMessage } from 'react-intl'; @@ -9,13 +10,14 @@ import messages from './messages'; import { Button } from '../button'; import { Alert } from '../alert'; import { DeleteModal } from '../deleteModal'; -import { styleClasses } from '../../views/projectEdit'; +import { styleClasses, StateContext } from '../../views/projectEdit'; import { fetchLocalJSONAPI, pushToLocalJSONAPI } from '../../network/genericJSONRequest'; import { useOnDrop } from '../../hooks/UseUploadImage'; import { useAsync } from '../../hooks/UseAsync'; import FileRejections from '../comments/fileRejections'; import DropzoneUploadStatus from '../comments/uploadStatus'; import { DROPZONE_SETTINGS } from '../../config'; +import { useFetch } from '../../hooks/UseFetch'; const ActionStatus = ({ status, action }) => { let successMessage = ''; @@ -360,22 +362,30 @@ const MessageContributorsModal = ({ projectId, close }: Object) => { ); }; -const TransferProject = ({ projectId }: Object) => { +const TransferProject = ({ projectId, orgId }: Object) => { const token = useSelector((state) => state.auth.get('token')); + const { projectInfo, } = useContext(StateContext); const [username, setUsername] = useState(''); - const [users, setUsers] = useState([]); - const handleUsers = (e) => { - const fetchUsers = (user) => { - fetchLocalJSONAPI(`users/?username=${user}&role=ADMIN`, token) - .then((res) => setUsers(res.users.map((user) => user.username))) - .catch((e) => setUsers([])); - }; - - const user = e.target.value; - setUsername(user); - fetchUsers(user); - }; + const [, loadingOptions, organisation] = useFetch(`organisations/${orgId}/?omitManagerList=false`) + + const options = organisation.managers?.map(({ username }) => ({ + label: username, + value: username, + })); + const handleSelect = (value) => { + setUsername(value); + }; + const { username: loggedInUsername, role: loggedInUserRole } = useSelector((state) => state.auth.get('userDetails')); + const hasAccess = ( + organisation.managers?.includes(loggedInUsername) || + loggedInUserRole === 'ADMIN' || + loggedInUsername === projectInfo.author + ); + const isDisabled = () => { + return ( + transferOwnershipAsync.status === 'pending' || !username || !hasAccess) + }; const transferOwnership = () => { return pushToLocalJSONAPI( `projects/${projectId}/actions/transfer-ownership/`, @@ -388,41 +398,23 @@ const TransferProject = ({ projectId }: Object) => { return (
- - } - on="focus" - position="bottom left" - open={users.length !== 0 ? true : false} + +
@@ -91,10 +91,9 @@ export const QuestionsAndComments = ({ projectId }) => { useEffect(() => { if (projectId && page) { - fetchLocalJSONAPI( - `projects/${projectId}/comments/?perPage=5&page=${page}`, - token, - ).then((res) => setComments(res)); + fetchLocalJSONAPI(`projects/${projectId}/comments/?perPage=5&page=${page}`, token).then( + (res) => setComments(res), + ); } }, [page, projectId, token]); diff --git a/frontend/src/components/projectEdit/actionsForm.js b/frontend/src/components/projectEdit/actionsForm.js index 95fc5b69f1..d5897262cb 100644 --- a/frontend/src/components/projectEdit/actionsForm.js +++ b/frontend/src/components/projectEdit/actionsForm.js @@ -363,52 +363,53 @@ const MessageContributorsModal = ({ projectId, close }: Object) => { const TransferProject = ({ projectId, orgId }: Object) => { const token = useSelector((state) => state.auth.get('token')); - const { projectInfo, } = useContext(StateContext); + const { projectInfo } = useContext(StateContext); const [username, setUsername] = useState(''); const [managers, setManagers] = useState([]); const [admins, setAdmins] = useState([]); const [isFetchingOptions, setIsFetchingOptions] = useState(true); useEffect(() => { - fetchLocalJSONAPI(`organisations/${orgId}/?omitManagerList=false`, token).then((r) => - setManagers(r.managers.map((m) => m.username))).then(() => - setIsFetchingOptions(false)); + fetchLocalJSONAPI(`organisations/${orgId}/?omitManagerList=false`, token) + .then((r) => setManagers(r.managers.map((m) => m.username))) + .then(() => setIsFetchingOptions(false)); fetchLocalJSONAPI(`users/?pagination=false`, token).then((t) => - setAdmins(t.users.map((u) => u.username))) + setAdmins(t.users.map((u) => u.username)), + ); }, [token, orgId]); const optionsExtended = [ { label: projectInfo.organisationName, - options: managers?.map(manager => ({ + options: managers?.map((manager) => ({ label: manager, value: manager, - })) + })), }, { label: , - options: admins?.filter( - admin => !managers?.includes(admin) - ).map(adminName => ({ - label: adminName, - value: adminName, - })) + options: admins + ?.filter((admin) => !managers?.includes(admin)) + .map((adminName) => ({ + label: adminName, + value: adminName, + })), }, ]; const handleSelect = (value) => { setUsername(value); }; - const { username: loggedInUsername, role: loggedInUserRole } = useSelector((state) => state.auth.get('userDetails')); - const hasAccess = ( + const { username: loggedInUsername, role: loggedInUserRole } = useSelector((state) => + state.auth.get('userDetails'), + ); + const hasAccess = managers?.includes(loggedInUsername) || loggedInUserRole === 'ADMIN' || - loggedInUsername === projectInfo.author - ); + loggedInUsername === projectInfo.author; const isDisabled = () => { - return ( - transferOwnershipAsync.status === 'pending' || !username || !hasAccess) + return transferOwnershipAsync.status === 'pending' || !username || !hasAccess; }; const transferOwnership = () => { return pushToLocalJSONAPI( @@ -428,11 +429,10 @@ const TransferProject = ({ projectId, orgId }: Object) => { getOptionLabel={({ label }) => label} getOptionValue={({ value }) => value} onChange={(e) => handleSelect(e?.value)} - value={optionsExtended?.find(manager => manager.value === username)} + value={optionsExtended?.find((manager) => manager.value === username)} options={optionsExtended} isLoading={isFetchingOptions} - > - + >
rel="noopener noreferrer" > - ; + +); diff --git a/frontend/src/components/projectEdit/settingsForm.js b/frontend/src/components/projectEdit/settingsForm.js index 20933ab42c..bf0f511a5c 100644 --- a/frontend/src/components/projectEdit/settingsForm.js +++ b/frontend/src/components/projectEdit/settingsForm.js @@ -127,11 +127,12 @@ export const SettingsForm = ({ languages, defaultLocale }) => {

- {(projectInfo.mappingEditors.includes('RAPID') || projectInfo.validationEditors.includes('RAPID')) && ( + {(projectInfo.mappingEditors.includes('RAPID') || + projectInfo.validationEditors.includes('RAPID')) && (
-