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

Journalize project attributes #11741

Merged

Conversation

cbliard
Copy link
Member

@cbliard cbliard commented Dec 1, 2022

@cbliard cbliard requested a review from a team December 1, 2022 10:09
@cbliard cbliard force-pushed the implementation/45081-journalize-project-attributes-changes branch from 03d0093 to 5dfb6db Compare December 1, 2022 10:15
@cbliard cbliard marked this pull request as draft December 1, 2022 10:36
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

I know this is not done yet. But I gave the code a glance over.

app/models/journal.rb Show resolved Hide resolved
@cbliard cbliard requested a review from a team December 2, 2022 10:42
@cbliard cbliard force-pushed the implementation/45081-journalize-project-attributes-changes branch 2 times, most recently from 4a39d12 to 50874b7 Compare December 2, 2022 16:54
@cbliard cbliard marked this pull request as ready for review December 2, 2022 16:58
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

General structure is looking good, @cbliard and what I have added are either smaller things or points triggered by the PR but not originating in it. For the later, we'd have to decide if they are valid and if so, when to do them.

@cbliard
Copy link
Member Author

cbliard commented Dec 5, 2022

@ulferts it's available for review when you have time.
Please do not merge as I'd like to squash some commits together.

cbliard added a commit that referenced this pull request Dec 5, 2022
In #11741 (comment)
Jens suggested to remove `activity_type` and rely on `journalized_type`
instead as there is a 1:1 relationship between them.

`activity_type` was introduced to differentiate between different types
of activity for the same journalized model (e.g. creation vs editing),
but this field is not used in practice.
cbliard added a commit that referenced this pull request Dec 6, 2022
In #11741 (comment)
Jens suggested to remove `activity_type` and rely on `journalized_type`
instead as there is a 1:1 relationship between them.

`activity_type` was introduced to differentiate between different types
of activity for the same journalized model (e.g. creation vs editing),
but this field is not used in practice.
@cbliard cbliard requested a review from ulferts December 6, 2022 09:43
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

My "Request for changes" is more a heads up (see below). The PR is good otherwise. Thanks for going the extra mile on my non related pointers.

@cbliard cbliard force-pushed the implementation/45081-journalize-project-attributes-changes branch from 3f50ff5 to ad45127 Compare December 6, 2022 13:48
@cbliard cbliard force-pushed the implementation/45081-journalize-project-attributes-changes branch from ad45127 to 2cc0a43 Compare December 6, 2022 14:08
@cbliard cbliard merged commit d318799 into dev Dec 6, 2022
@cbliard cbliard deleted the implementation/45081-journalize-project-attributes-changes branch December 6, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants