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

fix: using initial username as id and preventing later edits #1503

Merged
merged 3 commits into from
Oct 27, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/app/core/entity/model/entity.ts
Original file line number Diff line number Diff line change
@@ -139,12 +139,13 @@ export class Entity {
let indices = [];

// default indices generated from "name" property
if (this.hasOwnProperty("name")) {
if (typeof this["name"] === "string") {
indices = this["name"].split(" ");
}

return indices;
}

set searchIndices(value) {
// do nothing, always generated on the fly
// searchIndices is only saved to database so it can be used internally for database indexing
5 changes: 3 additions & 2 deletions src/app/core/permissions/ability/ability.service.spec.ts
Original file line number Diff line number Diff line change
@@ -212,8 +212,9 @@ describe("AbilityService", () => {
const userEntity = new User();
userEntity.name = user.name;
expect(ability.can("manage", userEntity)).toBeTrue();
userEntity.name = "another user";
expect(ability.cannot("manage", userEntity)).toBeTrue();
const anotherUser = new User();
anotherUser.name = "another user";
expect(ability.cannot("manage", anotherUser)).toBeTrue();
});

it("should allow to check conditions with complex data types", fakeAsync(() => {
11 changes: 10 additions & 1 deletion src/app/core/user/user.spec.ts
Original file line number Diff line number Diff line change
@@ -20,11 +20,20 @@ import { testEntitySubclass } from "../entity/model/entity.spec";

describe("User", () => {
testEntitySubclass("User", User, {
_id: "User:some-id",
_id: "User:tester",

name: "tester",
paginatorSettingsPageSize: {},

searchIndices: ["tester"],
});

it("should not allow to change the name after initialization and set it as the ID", () => {
const user = new User();
user.name = "test-name";

expect(user.name).toBe("test-name");
expect(user.getId()).toBe("test-name");
expect(() => (user.name = "another-name")).toThrowError();
});
});
18 changes: 17 additions & 1 deletion src/app/core/user/user.ts
Original file line number Diff line number Diff line change
@@ -34,7 +34,23 @@ export class User extends Entity {
label: $localize`:Label of username:Username`,
validators: { required: true },
})
name: string;
set name(value: string) {
Copy link
Member

Choose a reason for hiding this comment

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

let's maybe change the property to username (or will this break dozens of things?). For some clients we introduced additional firstname and lastname properties to be used in the UI. "username" would make the purpose of this one more explicit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the moment it would break a dozens of things (all the current user entities) So lets get this out and if we ever want to migrate this, we can do this independently.

if (this._name) {
// Throwing error if trying to change existing username
const label = User.schema.get("name").label;
throw new Error(
$localize`:Error message when trying to change the username|e.g. username cannot be changed after initialization:${label} cannot be changed after initialization`
Copy link
Member

Choose a reason for hiding this comment

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

best practices would suggest to not mix logic (the Entity model here) with UI (translating an error message), I think. The clean but more complex approach would be to define error codes or classes and only put human-readable and translated error messages when displaying it to the user. But I have my doubts that complexity is a worthy trade off here. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely a good thought. At the moment we are missing a general display error approach where we could translate these errors to a user friendly name.

);
}
this.entityId = value;
this._name = value;
}

get name(): string {
return this._name;
}

private _name: string;

/**
* settings for the mat-paginator for tables.