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

[SDESK-7464] - Create new async Users Resource and Service #2799

Open
wants to merge 4 commits into
base: async
Choose a base branch
from

Conversation

BrianMwangi21
Copy link

Purpose

This PR creates new async users resource and service:

  • Implementing a new resource model for users
  • Implementing new async resource service for users
  • Implementing resource config for users and register it in users module

Solves: SDESK-7464

@BrianMwangi21
Copy link
Author

@MarkLark86 a few questions :

  1. For areas with self.datasourse in the previous service, do we have a similar attribute for new async resources/models ?
  2. For the get function in the new users async service, should it be a custom one or should it override the find or search in the AsyncResourceService ?
  3. Is there a current equivalent to self._validator() in the new AsyncResourceService ? Or is it to be accessed by a resource model like. It's used in the DBUsersService on line 603 :
        validator = self._validator()  # TODO-ASYNC: Confirm on usage of validator

@MarkLark86 MarkLark86 added this to the 3.0 milestone Jan 7, 2025
@MarkLark86
Copy link
Contributor

@MarkLark86 a few questions :

  1. For areas with self.datasourse in the previous service, do we have a similar attribute for new async resources/models ?
  2. For the get function in the new users async service, should it be a custom one or should it override the find or search in the AsyncResourceService ?
  3. Is there a current equivalent to self._validator() in the new AsyncResourceService ? Or is it to be accessed by a resource model like. It's used in the DBUsersService on line 603 :
        validator = self._validator()  # TODO-ASYNC: Confirm on usage of validator
  1. self.datasource is the same as self.resource_name in the new async services
  2. Don't worry about the get function, this will be done on the REST API later
  3. Also don't worry about validator, as it's used by functions not required by this PR

superdesk/types/users.py Outdated Show resolved Hide resolved
superdesk/types/users.py Outdated Show resolved Hide resolved
superdesk/users/async_service.py Outdated Show resolved Hide resolved
superdesk/users/async_service.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MarkLark86 MarkLark86 left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple small things then this one should be good to go

@@ -0,0 +1,3 @@
from .users import UsersResourceModel
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a superdesk.tests file, which this module overrides causing an exception to be raised when running the app.

Could you move the content of that original file to this one, and delete the old one as well please

class UsersAsyncService(AsyncResourceService[UsersResourceModel]):
_updating_stage_visibility = True

async def __is_invalid_operation(
Copy link
Contributor

Choose a reason for hiding this comment

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

After further inspection, looks like this one is not needed here either, as it is in the context of an API request.

It's functions like these should not go into these new async resource services, but instead the REST API (when we implement the User one).

We want to keep the separation of concerns with the new code, by separating out the data and request layers specifically. This should help keep these two concerns smaller, easier to read etc

email = user_dict.get("email")
username = user_dict.get("username")
tokenDoc = {"user": user_id, "email": email}
id = resetService.store_reset_password_token(tokenDoc, email, activate_ttl, user_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

id shadows the built-in function with the same name. It would be good not to use names that are built into the python language

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants