feat(favorite_external_directory): add external directories favorite#3239
Conversation
… of the FavoriteExternalDirectory class
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| ) | ||
| result = self.session.execute(stmt) | ||
|
|
||
| return result.unique().scalar() |
There was a problem hiding this comment.
unique seems useless here ?
smailio
left a comment
smailio
left a comment
There was a problem hiding this comment.
Nice nice
I asked some questions and made some suggestions. Should be good after those are adressed.
| class FavoriteExternalDirectory(Base): | ||
| __tablename__ = "favorite_external_directory" | ||
|
|
||
| path: Mapped[str] = mapped_column(String, primary_key=True) | ||
| workspace: Mapped[str] = mapped_column(String, primary_key=True) | ||
| user_id: Mapped[int] = mapped_column(Integer, primary_key=True) | ||
|
|
||
| def to_dto(self) -> "FavoriteExternalDirectoryDTO": | ||
| return FavoriteExternalDirectoryDTO(path=PosixPath(self.path), workspace=self.workspace) |
There was a problem hiding this comment.
You should add an alembic migration for this
| study: Mapped["Study"] = relationship(Study) | ||
|
|
||
| def to_dto(self) -> FavoriteStudyDTO: | ||
| return FavoriteStudyDTO(study_id=self.study_id, study_name=self.study.name) |
There was a problem hiding this comment.
this is a hidden n+1 query btw
There was a problem hiding this comment.
It'll be a good ticket for a refactoring, thanks for telling me
| def __init__( | ||
| self, favorite_external_directory_repository: FavoriteExternalDirectoryRepository, workspace_config: Config | ||
| ): | ||
| self._favorite_external_directory_repository = favorite_external_directory_repository |
There was a problem hiding this comment.
why private member ? you didn't do this for sibling service
There was a problem hiding this comment.
It's an oversight on my part, I'll correct it. Thanks for the information.
| result = self.session.execute(stmt) | ||
| return list(result.scalars().all()) | ||
|
|
||
| def get(self, workspace: str, path: str) -> FavoriteExternalDirectory | None: |
There was a problem hiding this comment.
this get is only used in the delete service call. You can update the delete so that it checks the result.rowcount instead of doing a preventive check. You would save a db call most of the time a delete is called, and make the code base ligher.
| def list_favorite_external_directories( | ||
| favorite_external_directory_service: FavoriteExternalDirectoryServiceDep, | ||
| ) -> list[FavoriteExternalDirectoryDTO]: | ||
| logger.info("Listing favorite external directories for current user.") | ||
| return favorite_external_directory_service.list_favorites() |
There was a problem hiding this comment.
Ticket mentionned an aggregate endpoint that would return all type of directories, maybe that's coming in a future PR ?
There was a problem hiding this comment.
I do think it'll be in a coming PR, but I don't know when.
…vorite' into feat/add_external_directories_favorite
…vorite' into feat/add_external_directories_favorite
…vorite' into feat/add_external_directories_favorite
…vorite' into feat/add_external_directories_favorite
MartinBelthle
left a comment
MartinBelthle
left a comment
There was a problem hiding this comment.
1 change to be done inside the alembic migration, after this we can merge :)
| sa.Column('workspace', sa.String(length=255), nullable=False), | ||
| sa.Column('path', sa.String(length=255), nullable=False), | ||
| sa.Column('user_id', sa.Integer(), nullable=False), | ||
| sa.ForeignKeyConstraint(['user_id'], ['users.id'], ), |
There was a problem hiding this comment.
This should be mapped to identities.id instead of users.id, as you did in the model.py file
…vorite' into feat/add_external_directories_favorite
3 participants
What does it do ? : Implements the addition of external directories as favorite
Why it has been done ? : The external directories don't have a stable id, so we need to add their path (workspace + current path) in the favorite handling API
How to test it ? : Try to add external directories with valid / invalid workspace or paths