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

[Fixes #9550] Show creator/editor in 'Edit properties' #333

Merged
merged 7 commits into from
Feb 2, 2024

Conversation

afabiani
Copy link
Member

@afabiani afabiani commented Jan 17, 2024

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Suggestions

  • editor is misleading (and also creator, they both looks like some sort of roles), I'd suggest createdBy and lastUpdatedBy , consistent with creation and lastUpdate fields already present.

Issues

  • I tried to create a resource as user, than make it editable by everyone and edit as otheruser but the editor still remain user, instead it should become otheruser
  • This point is not respected.

    For existing resources we know only the owner, therefore creator/editor will be the same: this should be expected by the migration script itself

I'm admin and I can not see it in the ShortResource

image

While is visible for a new resource:

image

Anyway, I can say that while for creation we are sure about the role of the user, for last editing we are not sure. what do you think @tdipisa ?
About this point, as specified in my other review here , installation should be as much as possible automatic, without bore the users (and us too) to run also update scripts. So I suggest to make it programmatically (or not do it at all, this information was not registered before, so it makes sense to not have it).

@tdipisa
Copy link
Member

tdipisa commented Jan 19, 2024

@offtherailz I'm sorry but what do you mean with that?

Anyway, I can say that while for creation we are sure about the role of the user, for last editing we are not sure. what do you think @tdipisa ?

@tdipisa tdipisa linked an issue Jan 19, 2024 that may be closed by this pull request
2 tasks
@offtherailz
Copy link
Member

@tdipisa I wanted to suggest to not insert the "last updated by" information by checking the rule, because it is infact unknown.
Is better to not record this information at all and on the UI say last edited by "unknown"

@afabiani
Copy link
Member Author

@offtherailz It is responsability of the client setting the editor. How can the backend know who did the changes?

@offtherailz
Copy link
Member

@afabiani, I didn't followed this task from the beginning, but from my undestanding, this information is a typical "Created by", "Last modified by" metadata of the resource.
In every software I know, storing this kind of metadata (about who did what, when etc...) is responsability of the server, also for security reasons. Usually these kind of data are for administrators, and if someone did something wrong (e.g. maliciously) the normal user be able to change this information (e.g. inserting a fake name).

The server knows the user who does the operation, because it applies also security to it.

@tdipisa
Copy link
Member

tdipisa commented Jan 22, 2024

@afabiani

@tdipisa I wanted to suggest to not insert the "last updated by" information by checking the rule, because it is infact unknown.
Is better to not record this information at all and on the UI say last edited by "unknown"

As for the migration script it is ok for me if we persist only the creator.

For the rest I think you can proceed fixing what reported above in the review of @offtherailz

@afabiani
Copy link
Member Author

@tdipisa @offtherailz except for the name change suggested by Nali, the others changes have been done.

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Working perfectly 👍

@tdipisa
Copy link
Member

tdipisa commented Jan 26, 2024

@afabiani can you please solve conflicts? Thank you.

@afabiani
Copy link
Member Author

@offtherailz @tdipisa

  • conflicts fixed
  • sql script updated for the creator

@offtherailz offtherailz merged commit b3be458 into geosolutions-it:master Feb 2, 2024
2 checks passed
@afabiani afabiani deleted the ISSUE_9550 branch February 12, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show creator/editor in 'Edit properties'
3 participants