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.

Sign up for GitHub

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

Jump to bottom

Feature/58163 project specific stage administration #17200

Merged
merged 40 commits into from
Dec 16, 2024
Merged

Feature/58163 project specific stage administration #17200

merged 40 commits into from
Dec 16, 2024

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented Nov 13, 2024 •
edited
Loading

Ticket

https://community.openproject.org/wp/58163

What are you trying to accomplish?

  • A new menu is added to the project menu below the "Project settings" labeled "Project lifecycle"
  • It looks and behaves similar to the project attributes administration.
    • Individual Toggle
    • Bulk enable/disable
    • Filtering
  • To see and access the project administration a permission is required.
    • The permissions is called "Select project stages and gates".
    • Roles having the 'edit project' permission will receive the new permission both when seeding and when migrating.
  • Within this administration, all currently existing stages and gates are listed.
    • The name and type (stage/gate) is displayed.
      • The type (rhombus for gate, arrow for stage) are displayed in the color defined for the stage/gate.
    • They are listed in the order defined in the instance's administration.
      • The order cannot be changed.
  • The project administrator can activate and deactivate a gate/stage.
  • The new functionality is hidden behind the feature flag developed as part of ###58159

Out of scope from the original requirements as of now as it has not been implemented yet:

  • Disabled stages/gates will not be displayed:
    • In the overview page
    • In the project list (see ##58160)

Screenshots

image

Changes of note

  • The settings page helpers for specs are split up into individual files. Before, it was possible to go to the various pages by visit_tab!('backlogs'). The menu however no longer uses tabs and adding all the helpers to that one page led to some cluttering in there.
  • A component for displaying the type of a life cycle has been added and documented in the lookbook.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@ulferts ulferts changed the base branch from dev to feature/58159-fixed-set-of-project-stages-and-gates-editable-on-project-overview-page November 13, 2024 11:17
@dombesz dombesz force-pushed the feature/58159-fixed-set-of-project-stages-and-gates-editable-on-project-overview-page branch 3 times, most recently from 6775f44 to 63f8af7 Compare November 14, 2024 11:22
@ulferts ulferts force-pushed the feature/58163-project-specific-stage-administration branch 4 times, most recently from 4a407ed to 79c25d6 Compare November 22, 2024 07:43
@ulferts ulferts changed the base branch from feature/58159-fixed-set-of-project-stages-and-gates-editable-on-project-overview-page to feature/58159-rename-life-cycles November 22, 2024 13:46
@ulferts ulferts force-pushed the feature/58163-project-specific-stage-administration branch from 79c25d6 to c791037 Compare November 22, 2024 13:47
Base automatically changed from feature/58159-rename-life-cycles to feature/58159-fixed-set-of-project-stages-and-gates-editable-on-project-overview-page November 22, 2024 16:19
@dombesz dombesz force-pushed the feature/58159-fixed-set-of-project-stages-and-gates-editable-on-project-overview-page branch from f829684 to c485739 Compare November 22, 2024 16:58
Base automatically changed from feature/58159-fixed-set-of-project-stages-and-gates-editable-on-project-overview-page to dev November 25, 2024 08:31
@ulferts ulferts force-pushed the feature/58163-project-specific-stage-administration branch from c791037 to 36b37a4 Compare November 25, 2024 16:44
@ulferts ulferts force-pushed the feature/58163-project-specific-stage-administration branch 4 times, most recently from 12f662e to dc71f0b Compare November 27, 2024 09:41
@ulferts
Copy link
Contributor Author

ulferts commented Nov 28, 2024

The failing feature specs are flickering and weren't broken by the changes within this PR.

@ulferts ulferts marked this pull request as ready for review November 28, 2024 15:53
toy
toy reviewed Dec 6, 2024
View reviewed changes
Copy link
Contributor

@toy toy 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, mostly minor notes

app/components/projects/life_cycle_type_component.html.erb Show resolved Hide resolved
app/components/projects/settings/life_cycles/show_page_header_component.html.erb Outdated Show resolved Hide resolved
app/components/projects/settings/life_cycles/show_page_header_component.html.erb Outdated Show resolved Hide resolved
app/components/projects/settings/life_cycles/show_page_header_component.rb Outdated Show resolved Hide resolved
app/components/projects/settings/life_cycles/show_page_header_component.rb Outdated Show resolved Hide resolved
spec/support/pages/projects/settings/life_cycle.rb Outdated Show resolved Hide resolved
spec/support/pages/projects/settings/life_cycle.rb Outdated Show resolved Hide resolved
spec/features/projects/life_cycle/active_in_project_spec.rb Outdated Show resolved Hide resolved
spec/features/projects/life_cycle/active_in_project_spec.rb Outdated Show resolved Hide resolved
spec/features/projects/life_cycle/active_in_project_spec.rb Show resolved Hide resolved
@ulferts
Copy link
Contributor Author

ulferts commented Dec 9, 2024

Thanks for the thorough review, @toy . It sparked quite some improvements I think. I fear this lead to additional code to review.

I simply resolved all the conversations which I think to be addressed. Those where I am of a different opinion I left open and commented on.

@ulferts
Copy link
Contributor Author

ulferts commented Dec 9, 2024

The flickering spec is addressed by #17412

@ulferts
Copy link
Contributor Author

ulferts commented Dec 12, 2024

I worked in or commented on your remarks @toy. Please take another look.

@ulferts
Copy link
Contributor Author

ulferts commented Dec 13, 2024

Again, @toy, I would request a review from your side.

toy
toy approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@toy toy 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 to me

app/controllers/projects/settings/life_cycle_steps_controller.rb
definition = Project::LifeCycleStepDefinition.where(id: params[:definition_id])
definition = Project::LifeCycleStepDefinition.where(id: params[:id])
Copy link
Contributor

Choose a reason for hiding this comment

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

Very much nitpick - value is still an array, so there may be a bit of confusion why relation is assigned to variable with singular name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable name is correct in being singular since only a single element will ever be returned. So I find that appropriate. One could switch to using a find and wrap that into an array explicitly but that I find too cumbersome.

config/initializers/menus.rb
@@ -639,9 +643,9 @@

project_menu_items.each do |key, options|
menu.push :"settings_#{key}",
{ controller: "/projects/settings/#{key}", action: "show" },
{ controller: "/projects/settings/#{key}", action: "show" }.merge(options.slice(:action)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ controller: "/projects/settings/#{key}", action: "show" }.merge(options.slice(:action)),
{ controller: "/projects/settings/#{key}", action: "show", **options.slice(:action) },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find that harder to understand the the merge command.

config/locales/en.yml
@@ -2592,7 +2592,7 @@ en:
label_none_parentheses: "(none)"
label_not_contains: "doesn't contain"
label_not_equals: "is not"
label_life_cycle_plural: "Project lifecycle"
label_life_cycle_step_plural: "Project lifecycle"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label_life_cycle_step_plural: "Project lifecycle"
label_life_cycle_steps: "Project lifecycle"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that explicitly after noticing that a translation was no longer applied. I currently don't remember which component but there is a hard wired expectation that there is a label with a plural suffix. Otherwise I wouldn't even have put it into the way to nondescript label section.

config/routes.rb Outdated Show resolved Hide resolved
@ulferts ulferts merged commit 4971c8a into dev Dec 16, 2024
12 of 13 checks passed
@ulferts ulferts deleted the feature/58163-project-specific-stage-administration branch December 16, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toy toy toy approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ulferts @toy