diff --git a/backend/db/crud_question.py b/backend/db/crud_question.py index 0c6f1066..660edd3b 100644 --- a/backend/db/crud_question.py +++ b/backend/db/crud_question.py @@ -29,7 +29,9 @@ def get_question_by_commodity( res = [] for param in params: commodity = ( - session.query(Commodity).filter(Commodity.id == param["commodity"]).first() + session.query(Commodity) + .filter(Commodity.id == param["commodity"]) + .first() ) commodity = commodity.to_question_list commodity_category_id = commodity["commodity_category_id"] @@ -37,7 +39,8 @@ def get_question_by_commodity( session.query(Question) .join(CommodityCategoryQuestion) .filter( - CommodityCategoryQuestion.commodity_category == commodity_category_id, + CommodityCategoryQuestion.commodity_category + == commodity_category_id, ) .all() ) @@ -45,7 +48,8 @@ def get_question_by_commodity( if not param["breakdown"]: questions = list( filter( - lambda question: question["id"] == 1 or question["parent"] == 1, + lambda question: question["id"] == 1 + or question["parent"] == 1, questions, ) ) @@ -54,9 +58,13 @@ def get_question_by_commodity( return res -def get_question_by_case(session: Session, case_id: int) -> List[QuestionGroupListDict]: +def get_question_by_case( + session: Session, case_id: int +) -> List[QuestionGroupListDict]: case_commodity = ( - session.query(CaseCommodity).filter(CaseCommodity.case == case_id).all() + session.query(CaseCommodity) + .filter(CaseCommodity.case == case_id) + .all() ) commodities = ( session.query(Commodity) @@ -71,7 +79,8 @@ def get_question_by_case(session: Session, case_id: int) -> List[QuestionGroupLi session.query(Question) .join(CommodityCategoryQuestion) .filter( - CommodityCategoryQuestion.commodity_category == commodity_category_id, + CommodityCategoryQuestion.commodity_category + == commodity_category_id, ) .all() ) @@ -93,3 +102,9 @@ def get_question_by_case(session: Session, case_id: int) -> List[QuestionGroupLi } ) return res + + +def get_question_by_created_by(session: Session, created_by=int): + return ( + session.query(Question).filter(Question.created_by == created_by).all() + ) diff --git a/backend/db/crud_reference_data.py b/backend/db/crud_reference_data.py index 9ae91181..af3e1811 100644 --- a/backend/db/crud_reference_data.py +++ b/backend/db/crud_reference_data.py @@ -171,3 +171,11 @@ def delete_reference(session: Session, id: int): session.delete(data) session.commit() session.flush() + + +def get_reference_by_created_by(session: Session, created_by: int): + return ( + session.query(ReferenceData) + .filter(ReferenceData.created_by == created_by) + .all() + ) diff --git a/backend/models/question.py b/backend/models/question.py index a5593aa3..cd7bf466 100644 --- a/backend/models/question.py +++ b/backend/models/question.py @@ -53,10 +53,14 @@ class Question(Base): text = Column(String, nullable=False) description = Column(String, nullable=True) default_value = Column(String, nullable=True) - created_by = Column(Integer, ForeignKey("user.id"), nullable=True, default=None) + created_by = Column( + Integer, ForeignKey("user.id"), nullable=True, default=None + ) children = relationship("Question") - parent_detail = relationship("Question", remote_side=[id], overlaps="children") + parent_detail = relationship( + "Question", remote_side=[id], overlaps="children" + ) created_by_user = relationship( User, cascade="all, delete", passive_deletes=True, backref="questions" ) diff --git a/backend/models/reference_data.py b/backend/models/reference_data.py index 6d9afb21..dc601863 100644 --- a/backend/models/reference_data.py +++ b/backend/models/reference_data.py @@ -137,25 +137,25 @@ def __init__( year: str, source: str, link: str, - notes: Optional[str], - confidence_level: Optional[str], - range: Optional[str], - area: Optional[str], - volume: Optional[str], - price: Optional[str], - cost_of_production: Optional[str], - diversified_income: Optional[str], - area_size_unit: Optional[str], - volume_measurement_unit: Optional[str], - cost_of_production_unit: Optional[str], - diversified_income_unit: Optional[str], - price_unit: Optional[str], - type_area: Optional[str], - type_volume: Optional[str], - type_price: Optional[str], - type_cost_of_production: Optional[str], - type_diversified_income: Optional[str], - created_by: Optional[str], + notes: Optional[str] = None, + confidence_level: Optional[str] = None, + range: Optional[str] = None, + area: Optional[str] = None, + volume: Optional[str] = None, + price: Optional[str] = None, + cost_of_production: Optional[str] = None, + diversified_income: Optional[str] = None, + area_size_unit: Optional[str] = None, + volume_measurement_unit: Optional[str] = None, + cost_of_production_unit: Optional[str] = None, + diversified_income_unit: Optional[str] = None, + price_unit: Optional[str] = None, + type_area: Optional[str] = None, + type_volume: Optional[str] = None, + type_price: Optional[str] = None, + type_cost_of_production: Optional[str] = None, + type_diversified_income: Optional[str] = None, + created_by: Optional[int] = None, id: Optional[int] = None, ): self.id = id diff --git a/backend/routes/user.py b/backend/routes/user.py index c9807a2c..0610946a 100644 --- a/backend/routes/user.py +++ b/backend/routes/user.py @@ -5,6 +5,8 @@ import db.crud_case as crud_case import db.crud_user_case_access as crud_uca import db.crud_tag as crud_tag +import db.crud_question as crud_question +import db.crud_reference_data as crud_ref_data from math import ceil from middleware import ( @@ -444,19 +446,22 @@ def delete( if cases_owned_by_user: raise HTTPException( status_code=status.HTTP_409_CONFLICT, - detail=f"user {id} owned a case", + detail={ + "id": user.id, + "email": user.email, + "cases": [case.to_dropdown for case in cases_owned_by_user], + }, ) - # Ensure that the 'case updated_by' field is moved into the case owner. + # Ensure that the case updated by deleted user also clean + # by move created_by value into updated_by column cases_updated_by_user = crud_case.get_case_by_updated_by( session=session, updated_by=user.id ) if cases_updated_by_user: for case in cases_updated_by_user: case.updated_by = case.created_by - session.commit() - session.flush() - session.refresh(case) + session.commit() # Confirm that the user's case access is removed. crud_uca.delete_case_access_by_user_id(session=session, user_id=user.id) @@ -471,14 +476,29 @@ def delete( if tags: for tag in tags: tag.created_by = None - session.commit() - session.flush() - session.refresh(tag) + session.commit() - # question created by user? - # reference data created by user? + # Verify user's ID removed from questions created_by the user + questions = crud_question.get_question_by_created_by( + session=session, created_by=user.id + ) + if questions: + for question in questions: + question.created_by = None + session.commit() + + # Verify user's ID removed from reference created_by the user + references = crud_ref_data.get_reference_by_created_by( + session=session, created_by=user.id + ) + if references: + for reference in references: + reference.created_by = None + session.flush() + session.commit() + session.refresh(reference) - # delete user + # finally delete user crud_user.delete_user(session=session, id=user.id) return Response(status_code=HTTPStatus.NO_CONTENT.value) diff --git a/backend/tests/test_1010_user_deletion.py b/backend/tests/test_1010_user_deletion.py index b10731ad..7781cbe2 100644 --- a/backend/tests/test_1010_user_deletion.py +++ b/backend/tests/test_1010_user_deletion.py @@ -10,6 +10,9 @@ from models.user_case_access import UserCaseAccess from models.user_business_unit import UserBusinessUnit from models.tag import Tag +from models.question import Question +from models.reference_data import ReferenceData +from models.user import User pytestmark = pytest.mark.asyncio @@ -41,6 +44,16 @@ async def test_deleting_a_user_who_has_cases( headers={"Authorization": f"Bearer {account.token}"}, ) assert res.status_code == 409 + res = res.json() + assert res == { + "detail": { + "id": 1, + "email": "super_admin@akvo.org", + "cases": [ + {"label": "Bali Coffee Production (Private)", "value": 2} + ], + } + } @pytest.mark.asyncio async def test_deleting_a_user_who_doesnt_have_cases( @@ -100,6 +113,38 @@ async def test_deleting_a_user_who_doesnt_have_cases( assert tag is not None assert tag.created_by == deleted_user + # add question + question_id = 1000 + question = Question( + id=question_id, text="Test Question 1000", created_by=deleted_user + ) + session.add(question) + session.commit() + session.flush() + session.refresh(question) + assert question is not None + assert question.created_by == deleted_user + + # add reference_data + reference_id = 10000 + reference = ReferenceData( + id=reference_id, + country=1, + commodity=1, + region="asia", + currency="usd", + year="2020", + source="source", + link="link", + created_by=deleted_user, + ) + session.add(reference) + session.commit() + session.flush() + session.refresh(reference) + assert reference is not None + assert reference.created_by == deleted_user + # delete user res = await client.delete( app.url_path_for("user:delete", user_id=deleted_user), @@ -132,3 +177,22 @@ async def test_deleting_a_user_who_doesnt_have_cases( # make sure tag created_by removed tag = session.query(Tag).filter(Tag.created_by == deleted_user).first() assert tag is None + + # make sure question created_by is None + question = ( + session.query(Question).filter(Question.id == question_id).first() + ) + assert question is not None + assert question.id == question_id + assert question.created_by is None + + # make sure reference data created by is None + reference_data = ( + session.query(ReferenceData) + .filter(ReferenceData.created_by == deleted_user) + .all() + ) + assert reference_data == [] + + user = session.query(User).filter(User.id == deleted_user).first() + assert user is None diff --git a/frontend/src/pages/admin/users/Users.js b/frontend/src/pages/admin/users/Users.js index e61c2237..7d802626 100644 --- a/frontend/src/pages/admin/users/Users.js +++ b/frontend/src/pages/admin/users/Users.js @@ -1,10 +1,10 @@ -import React, { useEffect, useState } from "react"; +import React, { useEffect, useState, useCallback } from "react"; import { ContentLayout, TableContent } from "../../../components/layout"; import { Link } from "react-router-dom"; -import { EditOutlined } from "@ant-design/icons"; +import { EditOutlined, DeleteTwoTone } from "@ant-design/icons"; import upperFirst from "lodash/upperFirst"; import { api } from "../../../lib"; -import { Checkbox, Select } from "antd"; +import { Button, Checkbox, Select, Space, Popconfirm, Modal, List } from "antd"; import { selectProps } from "../../cases/components"; import "./user.scss"; @@ -18,7 +18,7 @@ const defData = { const filterProps = { ...selectProps, - style: { width: window.innerHeight * 0.225 }, + style: { width: window.innerHeight * 0.175 }, }; const userRoleOptions = [ @@ -47,32 +47,87 @@ const Users = () => { const [data, setData] = useState(defData); const [showApprovedUser, setShowApprovedUser] = useState(true); const [role, setRole] = useState(null); + const [deleting, setDeleting] = useState([]); + const [modal, contextHolder] = Modal.useModal(); + + const fetchUser = useCallback( + ({ currentPage, search, showApprovedUser, role }) => { + setLoading(true); + let url = `user?page=${currentPage}&limit=${perPage}&approved=${showApprovedUser}`; + if (search) { + url = `${url}&search=${search}`; + } + if (role) { + url = `${url}&role=${role}`; + } + api + .get(url) + .then((res) => { + setData(res.data); + }) + .catch((e) => { + console.error(e.response); + const { status } = e.response; + if (status === 404) { + setData(defData); + } + }) + .finally(() => { + setLoading(false); + }); + }, + [] + ); useEffect(() => { - setLoading(true); - let url = `user?page=${currentPage}&limit=${perPage}&approved=${showApprovedUser}`; - if (search) { - url = `${url}&search=${search}`; - } - if (role) { - url = `${url}&role=${role}`; - } + fetchUser({ currentPage, search, showApprovedUser, role }); + }, [currentPage, search, showApprovedUser, role, fetchUser]); + + const handleDeleteUser = (user) => { + setDeleting((prev) => [...prev, user.id]); api - .get(url) - .then((res) => { - setData(res.data); + .delete(`user/${user.id}`) + .then(() => { + fetchUser({ currentPage, search, showApprovedUser, role }); }) .catch((e) => { - console.error(e.response); - const { status } = e.response; - if (status === 404) { - setData(defData); + const { status, data } = e.response; + if (status === 409) { + const { cases } = data.detail; + // show the case names and add a button "Go to cases" + modal.confirm({ + title: `Unable to delete user ${user.email}`, + content: ( +
+

The following cases are still owned by this user:

+ {item.label}} + /> +

+ Please assign new owners for these cases before deleting this + user. +

+
+ ), + okText: "Go to cases", + onOk: () => { + const URL = `/cases?owner=${user.email}`; + window.open(URL, "_blank"); + }, + cancelText: "Cancel", + }); } }) .finally(() => { - setLoading(false); + setDeleting((prev) => prev.filter((id) => id !== user.id)); }); - }, [currentPage, search, showApprovedUser, role]); + }; const columns = [ { @@ -114,9 +169,27 @@ const Users = () => { width: "5%", align: "center", render: (text, record) => ( - - - + + + + + handleDeleteUser(record)} + okText="Yes" + cancelText="No" + placement="leftBottom" + > +