Content

Delete work package action doesn't use ::WorkPackages::DestroyService

I'm trying to implement a feature for OpenProject: automatically mark work packages if they have non-completed predecessors. To avoid recursive db-requests on every search, I added a boolean column to the work package db table and try to keep it in sync when any package created/updated/destroyed. I have modified ::WorkPackages::UpdateService and ::WorkPackages::DestroyService to do all the needed work.

But it seems like ::WorkPackages::DestroyService is never called, when a work package is deleted using the frontend, WorkPackages::BulkController is used instead.

Is there any clever reason for that? Or is it some legacy thing (I've seen some todos about that in the code)?

Could you suggest me any easy way to refactor destroying code into API+service way?

Replies (6)

just checked it myself and you are right, the WorkPackages::DestroyService is not used at the moment. I wasn't aware of that myself.

At least when only deleting single work packages one should be able to use the service and in order to do that one would have to call DELETE /api/v3/work_packages/:id. The action already exists so it seems to be a negligence to not use the action. We are always interested in PRs :)

For deleting multiple work packages, it might be more work though. We put a lot of work into handling saving of the work packages more efficiently, which is why we are now using the services more often, but I'd guess that the service is not used yet when multiple work packages are deleted. Which is not to say that it wouldn't be possible to do it.

Thank you for your reply! I thing I managed to add calling DestroyService directly to the BulkController. It creates a bit of code duplication with the API code, but I have no idea how to change the frontend/routing to forward bulk-requests to API... Anyway, now I have almost finished the implementations of services for relations and only need to implement filtering by this extra column. I will make a pull request a bit later, when I'm finished.

Btw, what are the responsibilities of services in OpenProject? I can see that it is some Rails design pattern and in OpenProject they wrap database access into a transaction. Are there any other responsibilities on them?

And is there any documentation about writing unittests for OpenProject? I see some specs folder, which looks like a test, but I'm not sure about where to start...

(I'm sorry if my questions sound a bit dumb, I'm a C++ developer most of the time)

a tad of duplication is probably unavoidable as long as we have different services for when destroying multiple work packages as compared to when destroying but one, so don't stress yourself.

Services where introduced to shift towards having less business logic in the model layer. This means leaving the "Rails way" where you have the Active Record pattern baked into the model layer so you would normally have your business logic in there. But the models have become to burdened so we decided to move towards services.

One of the benefits of having a service object is having the ability to test business logic without db access as mocking the db layer does not change the object under test. Granted, tests become more complicated when mocking but they also become much faster which is an issue in rails especially. Please see the tests/specs of the destroy service for an example of service unit tests.

Thank you for your clarifications about the services and tests! I've found some docs about RSpec, I'll try to do some tests afterwards.

Right now I'm having a small issue with creating a query filter for this new boolean field. I have creates my own filter in a separate file (link below), it shows in the GUI, it filters the tasks, but I cannot save the query due to some model validations, and I cannot understand why... It seems like :boolean symbol is somehow involved.

It looks like :boolean filter type is handled incorrectly by the frontend. When I just select the "Yes" or "No" in the filter GUI, the parameter passed to the backend is 't' or 'f', but when I try to save a filter it passes 'true' or 'false'...

I'm wondering, what is the correct value for allowed_values field of the filter?