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

fix visibility scope of labor items #15479

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented May 6, 2024

Fixes the visible scope of LaborBudgetItem. It was broken in multiple places:

  • Unless a project was provided, no scope (nil) was returned. In effect, that lead to the visible scope being inactive most of the time.
  • If a project was returned, the scope failed since it tried to find a non existing projects_id (instead of project_id) column on budgets.
  • When trying to actually run the scope that was supposed to be returned, that failed since there is no project_id column on the labor_budget_items table but only on the budgets table.

The second flaw suggests, that no project was ever provided as that would have lead to hard exceptions. The PR thus removes the second parameter which also brings the visibility scope more in line with those on other models.

Fixes

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

@ulferts ulferts marked this pull request as ready for review May 6, 2024 14:00
@dombesz dombesz self-requested a review May 8, 2024 12:49
Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

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

The change looks good, thanks @ulferts! 👏
However, I am not entirely sure if the bugfix works as expected. I tried to reproduce the issue and the Budget list page seems to correctly display 0 for the "Planned budget" if the user has the permissions from the ticket. The details page still shows the amounts of the "Planned budget" and to my understanding it should show 0.

image image image

Please feel free to merge it, if this is the expected behaviour.

@ulferts ulferts force-pushed the bug/45834-users-who-are-not-allowed-to-see-hourly-rates-see-planned-and-booked-labor-costs-in-budgets branch from 5f28a1c to b0527dc Compare May 14, 2024 11:18
@ulferts ulferts changed the base branch from dev to release/14.1 May 14, 2024 11:19
@ulferts
Copy link
Contributor Author

ulferts commented May 14, 2024

@dombesz you were right, that part of the information wasn't hidden properly. That was not part of the originally described bug but it makes sense to fix the issues together. Edit: I was wrong, it was described as well and I overlooked it.

I refactored a bit to make the code clearer but the only changes apart from what you already reviewed are in #2e0e39aacf6cfb90816227cc701579e09c07d16e which was changed again in 2e0e39aacf6cfb90816227cc701579e09c07d16e for a simpler implementation. The rest is just moving code around.

Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

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

Nice job extracting the code into view components!👏
Everything works as expected, I have one question only.

When the user is allowed to view_own_hourly_rate?, and has some time logged, the "Available" column shows the exact amount spent by the user with a negative sign.

From my point of view this is okay since we are not showing anything that the user is not supposed to see. But is this the expected behaviour?

image

@dombesz dombesz merged commit ab4e9bf into release/14.1 May 16, 2024
11 checks passed
@dombesz dombesz deleted the bug/45834-users-who-are-not-allowed-to-see-hourly-rates-see-planned-and-booked-labor-costs-in-budgets branch May 16, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants