feat(get): get more infos from user (DEV-641) by jnussbaum · Pull Request #181 · dasch-swiss/dsp-tools

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

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

@@ -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

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

sonarqubecloud · 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

-            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

-
-    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

@@ -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

+                    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

-            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

-    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

+        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

+        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

+        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 BalduinLandolt and irinaschubert 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

Comment thread knora/dsplib/models/user.py Outdated

Comment thread knora/dsplib/models/user.py Outdated

irinaschubert reviewed Apr 11, 2022

View reviewed changes

BalduinLandolt approved these changes Apr 12, 2022

View reviewed changes

Comment thread knora/dsplib/models/user.py Outdated

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

		@@ -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

Conversation

jnussbaum commented Apr 7, 2022

Uh oh!

jnussbaum commented Apr 7, 2022

Uh oh!

irinaschubert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BalduinLandolt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Apr 19, 2022

Uh oh!

jnussbaum commented Apr 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BalduinLandolt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irinaschubert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants