-
Notifications
You must be signed in to change notification settings - Fork 2
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
eduid_user_cleaner skatteverket worker #314
Conversation
return TUserDbDocument(data) | ||
|
||
@classmethod | ||
def from_dict(cls, data: Mapping[str, Any]) -> "CacheUser": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stället för att göra -> "CacheUser"
så kan man göra from __future__ import annotations
och sen använda CacheUser
utan "". Tills vi kommer upp på python 3.11 då man kan köra med -> Self
.
class CacheUser(BaseModel): | ||
eppn: str | ||
created_ts: datetime = Field(default_factory=utc_now) | ||
next_run_ts: Optional[int] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Varför är inte next_run_ts
en datetime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jo, för att de är enklare att räkna epoch när det ändå handlar om sekunder.
""" | ||
if self.next_run_ts is None: | ||
return "" | ||
return datetime.utcfromtimestamp(self.next_run_ts).strftime("%Y-%m-%d %H:%M:%S") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strftime kan bytas ut mot .isoformat()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixat!
def save(self, queue_user: CacheUser) -> bool: | ||
"""Save a CacheUser object to the database.""" | ||
if self.exists(queue_user.eppn): | ||
logger.debug(f"User {queue_user.eppn} already exists in the queue") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"exists in the queue" -> "exists in the cache"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixat, det fanns massa ställen där det stod Queue ist. för cache.
self.start_mock_auth_api(access_token_value=access_token_value) | ||
|
||
self.mocked_users = respx.mock(base_url="http://localhost", assert_all_called=False) | ||
self.central_user_db = central_user_db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vet inte om mypy eller våra IDEer kommer att vara så imponerade av att vi sätter något på self här.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vscode brydde sig inte. Men tagit bort dom eftersom dom inte används utanför metoden.
return Meta.from_dict(data=res) | ||
|
||
def exist(self, cleaner_type: CleanerType) -> bool: | ||
"""Check if a user exists in the cache.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
samma kommentar som andra exists
"""All workers base class, for collective functionality""" | ||
|
||
def __init__( | ||
self, cleaner_type: CleanerType, identity_type: IdentityType, test_config: Optional[Mapping[str, Any]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vet inte om identity_type kommer att fungera för alla cleaner workers, men det kommer väl framtiden utvisa.
|
||
# stats | ||
self.stats = init_app_stats(self.config) | ||
self.stats.count(name="user_cleaner_stats") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
räknar antalet gånger en cleaner har blivit instansierad? borde vi inte i så fall räkna typ färdiga körningar eller något?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bra ide! fixat
|
||
self.shutdown_now = False | ||
|
||
self.worker_queue: "Queue[dict[str, Union[str,int]]]" = Queue(maxsize=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behövs dubbelfnuttarna?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dom behövdes inte.
def _add_to_made_changes(self) -> None: | ||
""" | ||
Helper to add to made_changes after a change. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
det känns som man lika gärna kan använda self.made_changes alternativt låta _add_to_made_changes ta antalet ändringar dom gjorts kanske?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plockade bort metoden "_add_to_made_changes", - den blev överflödig.
SonarCloud Quality Gate failed.
|
SonarCloud Quality Gate failed.
|
superseded by work done in pr #636 |
No description provided.