[#73104] Add Sprint column to work package table#22706
Conversation
e4ff513 to
ce40bf4
Compare
|
I think we should not merge this until #22740 is in, so that we don't cause any conflicts for the big clean up PR. I will have to remove some lines about the feature flag, then. But that will be easy. |
| # Since the property is called `sprint`, but the model_name is `agile_sprint`, there would | ||
| # otherwise be a mismatch. Avoid that: | ||
| :sprint | ||
| end |
There was a problem hiding this comment.
I personally dislike having code I would consider representation (in the API) within the model. I am aware that this is a preference, not a strict rule.
But maybe backlogs could register the mapping on the ResourceLinkGenerator in its engine.rb
There was a problem hiding this comment.
Hmm, yes it is indeed a bit odd, especially since our other models don't do it like this. I moved the name mapping into a patch like suggested.
| create(:user, | ||
| member_with_permissions: { | ||
| project => project_permissions, | ||
| another_project => another_project_permissions |
There was a problem hiding this comment.
You should add another shared sprint where the user lacks permissions in the sharing project.
There was a problem hiding this comment.
Added some test cases for this. Doing so uncovered an issue with the previous query. Fixed that 👍
|
|
||
| def projects_with_view_sprints_permissions | ||
| Project.allowed_to(User.current, :view_sprints).select(:id) | ||
| end |
There was a problem hiding this comment.
Currently, sprints shared with projects are not all visible. Only if the user has the :view_sprints permission in the sharing project will the sprint be visible.
There is already a visible scope for sprints. This can be used and can more or less replace the hand written subselect (at the cost of a way more complicated SQL the scope hides).
The check in the JOIN condition can then be simplified to visible_sprints.id = work_packages.sprint_id I believe.
90275fd to
efa5a0a
Compare
This is necessary to make the group_by work
We delegate the resource name to the object itself, that way we can override it in our record and no knowledge about the backlogs module is necessary in the core.
596b5d6 to
457affb
Compare
|
#22740 has been merged, feature flag references have been removed from this PR. Comments have been addressed 🤞 |
| def determine_path_method(record) | ||
| return :sprint if record.is_a?(Agile::Sprint) | ||
|
|
||
| super |
There was a problem hiding this comment.
I didn't mean to suggest writing a patch, @EinLama. What I had in mind was something like
class API::V3::Utilities::ResourceLinkGenerator
def self.register_mapping(klass, symbol)
...
end
end
class OpenProject::Backlogs::Engine
config.to_prepare do
API::V3::Utilities::ResourceLinkGenerator.register_mapping(Agile::Sprint, :sprint)
end
end
But this here will also do the trick.
| #{visible_sprints.to_sql} | ||
| ) AS visible_sprints | ||
| ON visible_sprints.id = work_packages.sprint_id | ||
| AND "projects"."id" IN (#{projects_with_view_sprints.select(:id).to_sql}) |
There was a problem hiding this comment.
The joining of projects is not necessary here. You can simply check against project_id:
AND "work_packages"."project_id" IN (#{projects_with_view_sprints.select(:id).to_sql})
There was a problem hiding this comment.
You could also join the projects_with_view_sprints which might make it a bit easier to read:
LEFT OUTER JOIN (#{projects_with_view_sprints.to_sql}) "view_sprints_projects"
ON "view_sprints_projects"."id" = "work_packages"."project_id"
LEFT OUTER JOIN (
#{visible_sprints.to_sql}
) AS visible_sprints
ON visible_sprints.id = work_packages.sprint_id
AND view_sprints_projects.id IS NOT NULL
Then again, having written it, maybe it is not easier to read.
There was a problem hiding this comment.
I replaced the condition to use work_packages.project_id 👍 Is that what you meant?
We cannot get rid of the additional join on the projects table at the top of the SQL statement due to similar reasons like the project phase definition. If that line/join is removed, we get the same error we got back then:
PG::UndefinedTable: ERROR: missing FROM-clause entry for table "projects" LINE 1: ...status_id" WHERE ((statuses.is_closed=FALSE) AND (projects.i... ^
| # directly. Without this second condition, a sprint that is shared *to* | ||
| # project_receiving could leak into the sort/group for work packages that live in | ||
| # the sharer project itself, because `sprint_source_for` transitively includes the | ||
| # sharer when the user only has permission in the receiver. |
There was a problem hiding this comment.
Good catch here. Took me a while to understand but makes sense.
Ticket
https://community.openproject.org/wp/73104
What are you trying to accomplish?
Allows you to select the sprint column within the work package table and sort and group it.
Requires the
view_sprintspermission in a project to be available.Screenshots
What approach did you choose and why?
There are some notable points here:
Agile::Sprintwould be converted toagile_sprint, butsprintis correct. So we have to add a mapping. Since the core should not be required to know about the backlogs module, I added a way for models to provide their API resource link name. The given name will be used by the mapping if available.view_sprintspermission, a custom join was necessary - I think. The solution is similar to Implementation/62982 project phase column on work package list #18851, although this might be a case of "if I have a hammer, every problem looks like a nail". On the upside, this solution can be easily extended to work with further visibility checks in the future 👍Merge checklist