diff --git a/pydatalab/src/pydatalab/routes/v0_1/items.py b/pydatalab/src/pydatalab/routes/v0_1/items.py index a9fdadf10..60628e021 100644 --- a/pydatalab/src/pydatalab/routes/v0_1/items.py +++ b/pydatalab/src/pydatalab/routes/v0_1/items.py @@ -205,13 +205,9 @@ def creators_lookup() -> Dict: "from": "users", "let": {"creator_ids": "$creator_ids"}, "pipeline": [ - { - "$match": { - "$expr": { - "$in": ["$_id", {"$ifNull": ["$$creator_ids", []]}], - }, - } - }, + {"$match": {"$expr": {"$in": ["$_id", {"$ifNull": ["$$creator_ids", []]}]}}}, + {"$addFields": {"__order": {"$indexOfArray": ["$$creator_ids", "$_id"]}}}, + {"$sort": {"__order": 1}}, {"$project": {"_id": 1, "display_name": 1, "contact_email": 1}}, ], "as": "creators", @@ -639,6 +635,17 @@ def update_item_permissions(refcode: str): if creator.get("immutable_id", None) is not None ] + if not creator_ids: + return ( + jsonify( + { + "status": "error", + "message": "No valid creator IDs found in the request.", + } + ), + 400, + ) + # Validate all creator IDs are present in the database found_ids = [d for d in flask_mongo.db.users.find({"_id": {"$in": creator_ids}}, {"_id": 1})] # type: ignore if not len(found_ids) == len(creator_ids): @@ -663,11 +670,15 @@ def update_item_permissions(refcode: str): # The first ID in the creator list takes precedence; always make sure this is included to avoid orphaned items if current_creator_ids: base_owner = current_creator_ids[0] - try: - creator_ids.remove(base_owner) - except ValueError: - pass - creator_ids.insert(0, base_owner) + try: + creator_ids.remove(base_owner) + except ValueError: + pass + creator_ids.insert(0, base_owner) + + if set(creator_ids) == set(current_creator_ids): + # Short circuit if the creator IDs are the same + return jsonify({"status": "success"}), 200 LOGGER.warning("Setting permissions for item %s to %s", refcode, creator_ids) result = flask_mongo.db.items.update_one( @@ -676,7 +687,12 @@ def update_item_permissions(refcode: str): ) if not result.modified_count == 1: - return jsonify({"status": "error", "message": "Failed to update permissions"}), 400 + return jsonify( + { + "status": "error", + "message": "Failed to update permissions: you cannot remove yourself or the base owner as a creator.", + } + ), 400 return jsonify({"status": "success"}), 200 diff --git a/pydatalab/tests/server/test_permissions.py b/pydatalab/tests/server/test_permissions.py index c1c384a49..b149d958a 100644 --- a/pydatalab/tests/server/test_permissions.py +++ b/pydatalab/tests/server/test_permissions.py @@ -73,7 +73,9 @@ def test_basic_permissions_update(admin_client, admin_user_id, client, user_id): assert response.status_code == 200 # Check that the admin can remove the user from the permissions - response = admin_client.patch(f"/items/{refcode}/permissions", json={"creators": []}) + response = admin_client.patch( + f"/items/{refcode}/permissions", json={"creators": [{"immutable_id": str(admin_user_id)}]} + ) assert response.status_code == 200 response = client.get(f"/items/{refcode}") diff --git a/webapp/cypress/e2e/editPage.cy.js b/webapp/cypress/e2e/editPage.cy.js index 39f77e44b..17e76c6b5 100644 --- a/webapp/cypress/e2e/editPage.cy.js +++ b/webapp/cypress/e2e/editPage.cy.js @@ -205,12 +205,12 @@ describe("Edit Page", () => { cy.findByText("editable_sample").click(); cy.findByText("Add a block").click(); - cy.get(".dropdown-menu").findByText("Comment").click(); + cy.get('[data-testid="add-block-dropdown"]').findByText("Comment").click(); cy.contains("Unsaved changes").should("not.exist"); cy.findByText("Add a block").click(); - cy.get(".dropdown-menu").findByText("Comment").click(); + cy.get('[data-testid="add-block-dropdown"]').findByText("Comment").click(); cy.contains("Unsaved changes").should("not.exist"); @@ -258,7 +258,6 @@ describe("Edit Page", () => { cy.findByText("Add files from server...").click(); cy.findByText("Select files to add").should("exist"); - cy.findAllByLabelText("Close").eq(1).click(); }); it("Uploads an XRD file, makes an XRD block and checks that the plot works", () => { @@ -268,7 +267,7 @@ describe("Edit Page", () => { cy.findByText("editable_sample").click(); cy.findByText("Add a block").click(); - cy.get(".dropdown-menu").findByText("Powder XRD").click(); + cy.get('[data-testid="add-block-dropdown"]').findByText("Powder XRD").click(); cy.findByText("Select a file:").should("exist"); cy.get("select.file-select-dropdown").select("example_data_XRD_example_bmb.xye"); @@ -285,7 +284,7 @@ describe("Edit Page", () => { cy.findByText("editable_sample").click(); cy.findByText("Add a block").click(); - cy.get(".dropdown-menu").findByText("Media").click(); + cy.get('[data-testid="add-block-dropdown"]').findByText("Media").click(); cy.findAllByText("Select a file:").eq(1).should("exist"); cy.get("select.file-select-dropdown").eq(1).select(test_fname); diff --git a/webapp/src/components/ToggleableCreatorsFormGroup.vue b/webapp/src/components/ToggleableCreatorsFormGroup.vue index 2f4d4fc54..5c1be94f9 100644 --- a/webapp/src/components/ToggleableCreatorsFormGroup.vue +++ b/webapp/src/components/ToggleableCreatorsFormGroup.vue @@ -37,6 +37,7 @@ import Creators from "@/components/Creators"; import UserSelect from "@/components/UserSelect"; import { OnClickOutside } from "@vueuse/components"; import { updateItemPermissions } from "@/server_fetch_utils.js"; +import { toRaw } from "vue"; export default { components: { @@ -71,23 +72,28 @@ export default { }, }, watch: { - isEditingCreators(newValue, oldValue) { + async isEditingCreators(newValue, oldValue) { // Check we are leaving the editing state if (newValue === false && oldValue === true) { // Check that the permissions have actually changed - if (this.shadowValue === this.value) { + if (JSON.stringify(toRaw(this.value)) === JSON.stringify(toRaw(this.shadowValue))) { return; } try { + // Check if the user has just removed all creators and reset if so + if (this.shadowValue.length == 0) { + throw new Error("You must have at least one creator."); + } let answer = confirm("Are you sure you want to update the permissions of this item?"); if (answer) { - updateItemPermissions(this.refcode, this.shadowValue); + await updateItemPermissions(this.refcode, this.shadowValue); this.$emit("update:modelValue", [...this.shadowValue]); } else { this.shadowValue = [...this.value]; } } catch (error) { // Reset value to original + alert("Error updating permissions: " + error); this.shadowValue = [...this.value]; } } diff --git a/webapp/src/components/UserSelect.vue b/webapp/src/components/UserSelect.vue index 255a01aff..dcf0d5ac3 100644 --- a/webapp/src/components/UserSelect.vue +++ b/webapp/src/components/UserSelect.vue @@ -4,6 +4,7 @@ v-model="value" :options="filteredUsers" multiple + :min="minimumOptions" label="immutable_id" :filterable="false" @search="debouncedAsyncSearch" @@ -37,6 +38,10 @@ export default { type: Array, default: () => [], }, + minimumOptions: { + type: Number, + default: 0, + }, }, emits: ["update:modelValue"], data() { diff --git a/webapp/src/server_fetch_utils.js b/webapp/src/server_fetch_utils.js index 312025a9e..544416914 100644 --- a/webapp/src/server_fetch_utils.js +++ b/webapp/src/server_fetch_utils.js @@ -510,17 +510,14 @@ export function addABlock(item_id, block_type, index = null) { export function updateItemPermissions(refcode, creators) { console.log("updateItemPermissions called with", refcode, creators); - fetch_patch(`${API_URL}/items/${refcode}/permissions`, { + return fetch_patch(`${API_URL}/items/${refcode}/permissions`, { creators: creators, - }) - .then(function (response_json) { - if (response_json.status === "error") { - throw new Error(response_json.message); - } - }) - .catch(function (error) { - throw new Error(error); - }); + }).then(function (response_json) { + if (response_json.status === "error") { + throw new Error(response_json.message); + } + return response_json; + }); } export function saveItem(item_id) { diff --git a/webapp/src/views/EditPage.vue b/webapp/src/views/EditPage.vue index b1239580e..988c66e69 100644 --- a/webapp/src/views/EditPage.vue +++ b/webapp/src/views/EditPage.vue @@ -4,7 +4,6 @@ class="navbar navbar-expand sticky-top navbar-dark py-0 editor-navbar" :style="{ backgroundColor: navbarColor }" > - {{ itemTypeEntry?.navbarName || "loading..." }}  |   @@ -28,6 +27,7 @@ v-if="blockInfoLoaded" v-show="isMenuDropdownVisible" class="dropdown-menu" + data-testid="add-block-dropdown" style="display: block" aria-labelledby="navbarDropdown" >