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

feat(interview): Interview fixes #1234

Merged
merged 13 commits into from
Feb 7, 2025
Merged

feat(interview): Interview fixes #1234

merged 13 commits into from
Feb 7, 2025

Conversation

jochenklar
Copy link
Member

This PR contains a set of (hopefully) last fixes and changes to the interview rework.

@MyPyDavid
Copy link
Member

wanna rename to Breadcrumb in here as well? rdmo/projects/assets/js/interview/components/main/Breadcrump.js

@jochenklar jochenklar marked this pull request as ready for review February 4, 2025 16:03
@jochenklar jochenklar requested a review from MyPyDavid February 4, 2025 16:03
@jochenklar jochenklar self-assigned this Feb 4, 2025
@jochenklar jochenklar added this to the RDMO 2.3.0 milestone Feb 4, 2025
def copy_project(project, site, owners):
from .models import Membership, Value # to prevent circular inclusion
def copy_project(instance, site, owners):
from .models import Membership, Project, Value # to prevent circular inclusion
Copy link
Member

Choose a reason for hiding this comment

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

this .import ... was already there but it's not so clean. It is only needed because of that get_value_path method which is imported from this utils.py in models.value.py and used by the Value.file_path method.
That function can also easily (in a quick check) be refactored into the models.value.py module and would make the imports normal at the top.
Should I add a refactor commit for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

As explained in the meeting utils.py should not import models module-wide.

Copy link
Member

Choose a reason for hiding this comment

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

app.utils can only import models from other_app and not from app.models?

Copy link
Member Author

Choose a reason for hiding this comment

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

only if other_app is "below" app in our made up apps hierarchy core < conditions|domain|...|views < projects.

Copy link
Member

Choose a reason for hiding this comment

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

ok, but in this case everything is imported only from projects right? 😏

Copy link
Member

@MyPyDavid MyPyDavid left a comment

Choose a reason for hiding this comment

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

I found only 2 typing bugs and have a refactor suggestion, for the rest I think it looks good! 💫

@jochenklar jochenklar requested a review from MyPyDavid February 6, 2025 12:41
@jochenklar
Copy link
Member Author

There were some other issues, but I think now I am done. Can I merge?

Copy link
Member

@MyPyDavid MyPyDavid left a comment

Choose a reason for hiding this comment

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

thanks very much!
I tested it out and couldn't find any issues anymore! 💫

@jochenklar jochenklar merged commit 13dd803 into 2.3.0 Feb 7, 2025
19 checks passed
@jochenklar jochenklar deleted the interview-fixes branch February 7, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants