Feature/73109 sorting and uniqueness constraint in inbox and sprints#22393
Feature/73109 sorting and uniqueness constraint in inbox and sprints#22393
Conversation
0c8178c to
c69b1b1
Compare
EinLama
left a comment
There was a problem hiding this comment.
I didn't manage to get through the entire PR today :/ So I will leave the remarks I found in hopes it is helpful.
| def rebuild_positions | ||
| @project.rebuild_positions | ||
| if OpenProject::FeatureDecisions.scrum_projects_active? | ||
| WorkPackages::RebuildPositionsService.new(project: @project).call |
There was a problem hiding this comment.
It would be nice to have a return value here that indicates success or failure. Right now, we always show the flash notice - even if this service call would fail.
This would additionally allow us to write a more unit-test-style spec for the service. But I don't see a way to do this easily, so this will remain a wish for now. Since this action is not expected to be called often, investing all this time seems to be not worth it.
| def draggable? | ||
| current_user.allowed_in_project?(:manage_sprint_items, project) | ||
| end |
modules/backlogs/app/services/work_packages/rebuild_positions_service.rb
Outdated
Show resolved
Hide resolved
05c51d1 to
0808582
Compare
0808582 to
1bf4266
Compare
The expectation has changed in the meantime, since some menu items were removed.
EinLama
left a comment
There was a problem hiding this comment.
It works great 🎉 At first, I forgot to run rebuild positions once. But after remembering to do so, dragging and dropping worked flawlessly. Code looks great, too. Really looking forward to the day the feature flag will be enforced. We can get rid of so much code 😉
I double checked that the old implementation with the disabled feature flag still works 👍
Theres' one issue: I noticed that moving a work package via the action menu throws an error:
No route matches [GET] "/projects/agile-test-project/sprints/21/stories/49/reorder"
Additionally, the RbStoriesController#reorder requires changes, as it currently only replaces the Backlogs::BacklogComponent for legacy Sprints, and not the Backlogs::SprintComponent yet. With the feature freeze approaching, this could be fixed in another (bug) PR. I am not sure whether we already have a separate ticket for this task.
Approving since everything else works nicely and this PR provides a lot of value even with the bug present. Consider writing a bug ticket or fixing the move-via-menu-bug right here. Whatever fits your schedule better 😁
| FROM ( | ||
| SELECT | ||
| id, | ||
| ROW_NUMBER() OVER ( |
There was a problem hiding this comment.
TIL about ROW_NUMBER, very handy for this use case 💡
2176534 to
0459379
Compare
Ticket
https://community.openproject.org/wp/73109
https://community.openproject.org/wp/73093
What are you trying to accomplish?
The original task was to get acts_as_list to work within a
project_id, sprint_idscope while keeping the oldproject_id, version_id, type = [backlogs types]scope working. For the sake of the new scope addingwould have sufficed. But because it needs to work also with the feature flag off, the solution requires overwriting some of acts_as_list's methods. All of this can be greatly simplified once the feature flag is removed.
A service was added that resets the position value on all work packages within a certain scope (i.e. project or every project). This can for now be called from the project administration and is necessary to get the positions set up correctly first.
Before feature freeze, a migration is necessary that calls the service. This is not included yet.
This initial scope is handled in the first three commits.
Along the way, a number of problems have been identified and are now fixed: