feat(get): get more infos from user (DEV-641) #181

jnussbaum · 2022-04-07T15:41:06Z

jnussbaum · 2022-04-07T16:09:24Z

Open concerns:

user.py, line 725: The method User.fromJsonObj() wants the project and the groups to be inside the user[permissions] as dict. It ignores the fields "groups" and "projects" in the API-retrieved user-JSON-objects. I suspect that User.fromJsonObj() should be adapted, but I don't really understand how. So I decided to make a "hack": I add the "groups" and "projects" two times: first to the API-retrieved object, and then to the models.user.User objects.
In the original onto, the ontos of the current file are not listed in the "prefixes" section (because we made them optional some weeks ago). But in the retrieved one, they are. In the GET-process, I think it would make sense to eliminate all prefixes of the current onto file. What do you think?
In users>groups and users>projects, The prefixes could be omitted as well.

irinaschubert

A question: how are users treated if they are members of several projects and members of groups of other projects, are these projects and groups retrieved as well? If so, should this information be provided in with the dsp-tools get command?

About your questions:

user.py, line 725: I'm not sure if I understand your question: The API route that is used on line 692 does not return the groups and projects, so you need to get the groups and users separately, like you did. What do you mean with "I add the groups and projects two times"? You only add them once to the user you retrieved from the API. Not sure if I miss something here...
Prefixes of the acutal ontology: I would leave them there. It is correct and it's a useful information. Or is there a reason why it shouldn't be in the ontology? --> I don't really have a strong opinion on that - do what you prefer
I would leave them there, to me they aren't disturbing. --> I don't really have a strong opinion on that - do what you prefer

knora/dsplib/models/user.py

irinaschubert · 2022-04-11T12:43:11Z

knora/dsplib/models/user.py

@@ -689,7 +690,7 @@ def getAllUsersForProject(con: Connection, proj_shortcode: str) -> list[Any]:

        result = con.get(User.ROUTE)


Now that I think about it, there is a route to get all users for a specific project, it's:
/admin/projects/shortcode/{{project_shortcode}}/members

It's probably easier to use this for getAllUsersForProject directly. There are also groups and projects for the user.

BalduinLandolt

I don't have much to add to this one, beyond the comments Irina has already made

knora/dsplib/models/user.py

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss> Co-authored-by: Balduin Landolt <33053745+BalduinLandolt@users.noreply.github.com>

sonarcloud · 2022-04-19T13:34:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

jnussbaum · 2022-04-19T13:43:12Z

Thanks to Irina's comment (#181 (comment)) I managed to considerably simplify the code and to resolve the concern I had in #181 (comment).

Besides, I implemented your feedback and I also did some refactorings where it seemed useful.

The PR has now changed considerably, so I ask you again for a review.

jnussbaum · 2022-04-19T13:45:02Z

knora/dsplib/models/user.py

-            project_groups = json_obj['permissions']['groupsPerProject']
-            for project in project_groups:
-                if project == Project.SYSTEM_PROJECT:
-                    if Group.PROJECT_SYSTEMADMIN_GROUP in project_groups[project]:
+            for project_iri, group_memberships in json_obj['permissions']['groupsPerProject'].items():
+                if project_iri == Project.SYSTEM_PROJECT:
+                    if Group.PROJECT_SYSTEMADMIN_GROUP in group_memberships:
                        sysadmin = True
                else:
-                    for group in project_groups[project]:
+                    for group in group_memberships:
                        if group == Group.PROJECT_MEMBER_GROUP:
-                            if in_projects.get(project) is None:
-                                in_projects[project] = False
+                            if in_projects.get(project_iri) is None:
+                                in_projects[project_iri] = False
                        elif group == Group.PROJECT_ADMIN_GROUP:
-                            in_projects[project] = True
+                            in_projects[project_iri] = True


This is just a refactoring, without semantic modification.

jnussbaum · 2022-04-19T13:46:23Z

knora/dsplib/models/user.py

-
-    def createDefinitionFileObj(self):
-        user = {
+        members = con.get(f'/admin/projects/shortcode/{proj_shortcode}/members')


With this route, the same task can be achieved with less code

BalduinLandolt

one can always find more to change and improve... but I think this should be good for now. (I added some minor remarks nonetheless)

BalduinLandolt · 2022-04-21T13:04:46Z

knora/dsplib/models/user.py

@@ -475,18 +476,17 @@ def fromJsonObj(cls, con: Connection, json_obj: Any):
        in_groups: set[str] = set()
        if json_obj.get('permissions') is not None and json_obj['permissions'].get('groupsPerProject') is not None:


generally, if x is not None can normally be rewritten as if not x (but that doesn't need to be done in this PR... I know it's all over the place in dsp-tools)

BalduinLandolt · 2022-04-21T13:06:21Z

knora/dsplib/models/user.py

+                    if Group.PROJECT_SYSTEMADMIN_GROUP in group_memberships:
                        sysadmin = True


this could be simplified even further (I think): sysadmin = (Group.PROJECT_SYSTEMADMIN_GROUP in group_memberships)

BalduinLandolt · 2022-04-21T13:07:16Z

knora/dsplib/models/user.py

-            project_groups = json_obj['permissions']['groupsPerProject']
-            for project in project_groups:
-                if project == Project.SYSTEM_PROJECT:
-                    if Group.PROJECT_SYSTEMADMIN_GROUP in project_groups[project]:
+            for project_iri, group_memberships in json_obj['permissions']['groupsPerProject'].items():
+                if project_iri == Project.SYSTEM_PROJECT:
+                    if Group.PROJECT_SYSTEMADMIN_GROUP in group_memberships:
                        sysadmin = True
                else:
-                    for group in project_groups[project]:
+                    for group in group_memberships:
                        if group == Group.PROJECT_MEMBER_GROUP:
-                            if in_projects.get(project) is None:
-                                in_projects[project] = False
+                            if in_projects.get(project_iri) is None:
+                                in_projects[project_iri] = False
                        elif group == Group.PROJECT_ADMIN_GROUP:
-                            in_projects[project] = True
+                            in_projects[project_iri] = True


BalduinLandolt · 2022-04-21T13:09:27Z

knora/dsplib/models/user.py

-    def createDefinitionFileObj(self):
-        user = {
+        members = con.get(f'/admin/projects/shortcode/{proj_shortcode}/members')
+        if members is None or len(members) < 1:


this can be simplified to if not members: (both None and empty lists/dicts/sets evaluate as false in an if-clause)

BalduinLandolt · 2022-04-21T13:12:47Z

knora/dsplib/models/user.py

+        if members is None or len(members) < 1:
+            return None
+        res: list[User] = [User.fromJsonObj(con, a) for a in members['members']]
+        res.reverse()


I'm not sure the order of the objects you get from the API is deterministic (at least if the cache is rebuilt), so this may not reliably reproduce the initial order. But if it works in the cases you tested, feel free to leave it in.

In order to find out if it works as expected (also in the future), you could add a test for this method.

BalduinLandolt · 2022-04-21T13:15:00Z

knora/dsplib/utils/onto_get.py

+        for usr in users:
+            users_obj.append(usr.createDefinitionFileObj(
+                con=con, proj_shortname=project.shortname, proj_shortcode=project.shortcode
+            ))
+            if verbose:
+                print(f"\tGot user '{usr.username}'")


is usr intentional?

no, rather random

irinaschubert

LGTM

irinaschubert · 2022-04-25T06:42:17Z

knora/dsplib/models/user.py

+        if members is None or len(members) < 1:
+            return None
+        res: list[User] = [User.fromJsonObj(con, a) for a in members['members']]
+        res.reverse()


In order to find out if it works as expected (also in the future), you could add a test for this method.

jnussbaum added 3 commits April 5, 2022 09:36

first attempt of problemsolving

e75c876

edit

0f3ee1c

works

34f046f

jnussbaum self-assigned this Apr 7, 2022

jnussbaum changed the title ~~get: get more infos from user (DEV-641)~~ feat(get): get more infos from user (DEV-641) Apr 7, 2022

jnussbaum requested review from irinaschubert and BalduinLandolt April 7, 2022 19:30

Merge branch 'main' into wip/dev-641-get-more-infos-from-user

3fa2b44

irinaschubert reviewed Apr 11, 2022

View reviewed changes

knora/dsplib/models/user.py Outdated Show resolved Hide resolved

knora/dsplib/models/user.py Outdated Show resolved Hide resolved

irinaschubert reviewed Apr 11, 2022

View reviewed changes

BalduinLandolt approved these changes Apr 12, 2022

View reviewed changes

knora/dsplib/models/user.py Outdated Show resolved Hide resolved

jnussbaum and others added 5 commits April 19, 2022 09:55

Apply suggestions from code review

0f6bc68

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss> Co-authored-by: Balduin Landolt <33053745+BalduinLandolt@users.noreply.github.com>

Merge branch 'main' into wip/dev-641-get-more-infos-from-user

6210b36

replace lambdas by comprehensions

7542c10

edit

c1362e4

inverse order of users

373bf93

jnussbaum commented Apr 19, 2022

View reviewed changes

jnussbaum requested a review from BalduinLandolt April 19, 2022 13:48

BalduinLandolt approved these changes Apr 21, 2022

View reviewed changes

jnussbaum merged commit 407f5c5 into main Apr 25, 2022

jnussbaum deleted the wip/dev-641-get-more-infos-from-user branch April 25, 2022 06:42

irinaschubert approved these changes Apr 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(get): get more infos from user (DEV-641) #181

feat(get): get more infos from user (DEV-641) #181

jnussbaum commented Apr 7, 2022

jnussbaum commented Apr 7, 2022

irinaschubert left a comment

irinaschubert Apr 11, 2022

BalduinLandolt left a comment

sonarcloud bot commented Apr 19, 2022

jnussbaum commented Apr 19, 2022

jnussbaum Apr 19, 2022

BalduinLandolt Apr 21, 2022

jnussbaum Apr 19, 2022

BalduinLandolt left a comment

BalduinLandolt Apr 21, 2022

BalduinLandolt Apr 21, 2022

BalduinLandolt Apr 21, 2022

BalduinLandolt Apr 21, 2022

BalduinLandolt Apr 21, 2022

irinaschubert Apr 25, 2022

BalduinLandolt Apr 21, 2022

jnussbaum Apr 25, 2022

irinaschubert left a comment

irinaschubert Apr 25, 2022

		@@ -689,7 +690,7 @@ def getAllUsersForProject(con: Connection, proj_shortcode: str) -> list[Any]:

		result = con.get(User.ROUTE)

		@@ -475,18 +476,17 @@ def fromJsonObj(cls, con: Connection, json_obj: Any):
		in_groups: set[str] = set()
		if json_obj.get('permissions') is not None and json_obj['permissions'].get('groupsPerProject') is not None:

		if Group.PROJECT_SYSTEMADMIN_GROUP in group_memberships:
		sysadmin = True

feat(get): get more infos from user (DEV-641) #181

feat(get): get more infos from user (DEV-641) #181

Conversation

jnussbaum commented Apr 7, 2022

jnussbaum commented Apr 7, 2022

irinaschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BalduinLandolt left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 19, 2022

jnussbaum commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BalduinLandolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irinaschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment