-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[#57688] Primerize Backlogs - CLASSIC #21723
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
base: dev
Are you sure you want to change the base?
Conversation
0a230ad to
5282199
Compare
5282199 to
b8fa4bb
Compare
b8fa4bb to
7a142b0
Compare
e1c2623 to
9e9c053
Compare
9e9c053 to
2777895
Compare
bebf10e to
25e2c19
Compare
25e2c19 to
7c350e7
Compare
| @@ -52,4 +98,10 @@ def load_sprint_and_project | |||
| # This overrides sprint's project if we set another project, say a subproject | |||
| @project = Project.find(params[:project_id]) if params[:project_id] | |||
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.
Memo to self: we could consider adding the visible scope here so that the controllers throws a 404 in case a user is not allowed to view a Project instead of a 403.
| def reorder | ||
| story = Story.find(params[:id]) | ||
|
|
||
| if story.update(move_to: reorder_param) |
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 think the permissions here are not implicitly verified for Story on work package level. Only on project level. So a user with access to the project can access any work package (=Story) here.
Since update is called directly here, the visibility-permission check that would happen if we used the Story service here is also bypassed.
We should either ensure the Controllers perform all necessary checks or rely on services to ensure this - or do both 😁
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.
Hey @myabc I really like where this is heading. Doesn't look like Y2K any more.
I didn't do a complete and thorough review but rather read through it all and remarked on things here and there.
There are a number of general topics I'd like to raise to discuss later:
- Currently, the implementation is based on turbo streams. Backlogs are loaded individually when they are changed which, even with the limited set of scenarios provided does not always work and leads to complicated code. I have never done it and don't know if it will work but I would be interested in your opinion on using turbo drive or frame to load the whole page. To avoid undesired flickering and state changes, morphing would need to be used and the state would need to be sent to the backend (or morphing disabled on some parts of the application via JS). This would greatly simplify the backend/frontend communication I believe. Reloading the page upon changes in the split view e.g. would not need a lot of knowledge on what was changed. With the current approach, on a change to version, two backlogs would need to be reloaded. For the move action, this already seems to be failing and that is only within the stimulus controlled parts of the application. E.g. when moving the last story out of a backlog, it receives a way to big "Sprint backlog is empty" skeleton. All the other empty sprints are updated as well. This can be fixed but I see that as a warning sign of things to come:
- As mentioned above, changes in the work package split view are not reflected in the backlog view. Additionally, one can change the work package in a way that it will disappear from the page. Currently this can be done by changing the type to a non story type or by changing the version to one that is not on the page. The first problem will likely disappear once the setting is removed.
- With the split view opening, there is very little space
Then there are some bugs I noticed when trying out the changes:
- Moving the a story to the backlog and trying to move it again right away does not work
- When opening the split view, the backlogs area sometimes did not allow to scroll horizontally. That happens after drag and drop, maybe?
- Opening the split view breaks the right hand side margin so backlog and button are glued to the right hand side:
- We might consider opening a folded backlog when dragging. At least after hovering over it for some time
- The menu item says "Edit title" when in fact I can edit more attributes:
|
|
||
| this.element.addEventListener('click', this, { signal }); | ||
| this.element.addEventListener('dblclick', this, { signal }); | ||
| this.element.addEventListener('keydown', this, { signal }); |
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.
Out of curiosity - why not use stimulus actions here. The construct used here seems overly complicated. But admittedly, I haven't tried using stimulus actions on elements like you are aiming for here.
| } else { | ||
| this.openSplitPane(); | ||
| } | ||
| } |
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.
If the approach is kept, this part of the code needs to be dry-ed
| blankslate.with_heading(tag: :h4).with_content(t(".product_backlog.blank_slate_title")) | ||
| blankslate.with_description_content(t(".product_backlog.blank_slate_description")) | ||
| end | ||
| %> |
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.
Moving the logic to the component (backlog.sprint_backlog?) would simplify the template since the only difference seems to be in the I18n keys.
|
|
||
| registerCustomElement('opce-my-page', MyPageComponent, { injector }); | ||
| registerCustomElement('opce-dashboard', DashboardComponent, { injector }); | ||
| registerCustomElement('opce-burndown-chart', BurndownChartComponent, { injector }); |
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.
Is there a way to keep it in the backlogs module?
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.
@ulferts good question. That would be tidier. Let me ask @oliverguenther !
|
|
||
| types | ||
| end | ||
| end |
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.
This is the code snippet needed for getting the right story type for when creating a new one via the modal
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.
@ulferts ok. available_story_types.first is now passed to the modal.
| mem | ||
| end | ||
| end | ||
|
|
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.
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.
@ulferts ah, good catch.
| backlog_component: | ||
| sprint_backlog: | ||
| blank_slate_title: "Sprint backlog is empty" | ||
| blank_slate_description: "No items planned for this Sprint" |
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.
Why is sprint written in uppercase here?
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.
@ulferts let's discuss this with @psatyal perhaps. We are inconsistent here.
Also: https://chatgpt.com/share/697bacd2-7078-8006-b65a-353e12724dfa
7c350e7 to
e8486ab
Compare
e8486ab to
e61b486
Compare
modules/backlogs/app/components/backlogs/backlog_component.html.erb
Outdated
Show resolved
Hide resolved
modules/backlogs/app/components/backlogs/collapsible_component.html.erb
Outdated
Show resolved
Hide resolved
e61b486 to
4164658
Compare
4164658 to
1611283
Compare
1611283 to
c0e8332
Compare
cbc8ddb to
495075f
Compare
modules/backlogs/app/components/backlogs/backlog_header_component.rb
Outdated
Show resolved
Hide resolved
modules/backlogs/app/components/backlogs/backlog_menu_component.html.erb
Outdated
Show resolved
Hide resolved
| project, | ||
| version_id: sprint.id, | ||
| story_points: 0, | ||
| type_id: Story.types.first |
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.
@ulferts I guess we should choose between:
- leaving as-is for this iteration (this will involve disabling various features)
- creating a custom Add Story modal.
- making the existing Create work Package modal configurable**.
In terms of amount of work, I would guesstimate that it's in that order.
| }, | ||
| data: { | ||
| target: "collapsible-header.triggerElement", | ||
| action: "click:collapsible-header#toggle keydown:collapsible-header#toggleViaKeyboard" |
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.
|
|
||
| registerCustomElement('opce-my-page', MyPageComponent, { injector }); | ||
| registerCustomElement('opce-dashboard', DashboardComponent, { injector }); | ||
| registerCustomElement('opce-burndown-chart', BurndownChartComponent, { injector }); |
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.
@ulferts good question. That would be tidier. Let me ask @oliverguenther !
modules/backlogs/app/components/backlogs/collapsible_component.html.erb
Outdated
Show resolved
Hide resolved
| backlog_component: | ||
| sprint_backlog: | ||
| blank_slate_title: "Sprint backlog is empty" | ||
| blank_slate_description: "No items planned for this Sprint" |
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.
@ulferts let's discuss this with @psatyal perhaps. We are inconsistent here.
Also: https://chatgpt.com/share/697bacd2-7078-8006-b65a-353e12724dfa
| mem | ||
| end | ||
| end | ||
|
|
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.
@ulferts ah, good catch.
b38b08a to
861e1cb
Compare
| <details> | ||
| <summary>Debug</summary> | ||
| <code> | ||
| <pre>{{maxValue() }}</pre> | ||
| <pre>{{lineChartData() | json}}</pre> | ||
| </code> | ||
| </details> |
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.
TODO: remove this (or only show in dev)
| import PrimerColorsPlugin from 'core-app/shared/components/work-package-graphs/plugin.primer-colors'; | ||
| import { BaseChartDirective, provideCharts, withDefaultRegisterables } from 'ng2-charts'; | ||
|
|
||
| const BURNDOWN_Y_SCALE_MIN = 25; |
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.
@opf/polaris-developers FYI - this magic number is taken from BurndownChartHelper:
def yaxis_labels(burndown)
max = burndown.max[:points]
mvalue = (max / 25) + 1
labels = (0..mvalue).map { |i| "[#{i * 25}, #{i * 25}]" }
mvalue = mvalue + 1 if mvalue == 1 || ((max % 25) == 0)
labels << "[#{mvalue * 25}, '<span class=\"axislabel\">#{I18n.t('backlogs.points')}</span>']"
result = labels.join(", ")
result.html_safe
endDisplay on multiple lines on smaller viewports.
Updates `SprintPageHeaderComponent` to support `with_action_button`.
#21814) * Initial plan * Use fetch_or_fallback to validate state in BacklogHeaderComponent Co-authored-by: myabc <[email protected]> * Add test for state validation fallback behavior Co-authored-by: myabc <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: myabc <[email protected]>
861e1cb to
d6f9758
Compare

Ticket
https://community.openproject.org/wp/57688
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist