-
Notifications
You must be signed in to change notification settings - Fork 19
fix: Use DI for getting 3rdparty app classes #502
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
Conversation
|
Not sure about the CI failures, seem to also be the case on the master branch |
yes, that's related to psalm unrelated to this PR changes. |
|
@juliushaertl Thank you for the PR. Could you please base the PR against the branch |
061ee0b to
6485232
Compare
6485232 to
2417789
Compare
|
@wielinde Done. Now based on |
|
I also amended a bit more cleanup of properties that are no longer needed. |
2417789 to
edfad53
Compare
|
I've pushed a commit to fix the unit tests 6b51527 |
6b51527 to
58cb1be
Compare
|
@SwikritiT did anybody also test this PR on the older, supported Nextcloud versions? If yes, please release it as a patch level release |
We run the tests on all the supported versions and the API tests are passing in all of them. But we can do a quick manual test on all the versions as well |
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Swikriti Tripathi <[email protected]>
58cb1be to
6588c72
Compare
JS Code CoverageCoverage after merging bugfix/noid/27.1.2-activity-classes into release/2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Did the manual test and everything works on nextcloud side, in all the supported versions. I'll merge this PR and release the patch level |
Cool. Thank you for the QA. One question out of curiosity: Did our CI fail when Nextcloud released 27.1.2? I mean, did it fail before this PR? I guess that the CI only runs on changes in the app. So if Nextcloud has a new stable release, that makes our CI fail, then we will not notice until our next commit to the app, which might be too late. Maybe we need to pick up on changes on Nextcloud stable branches, too? |
Yes, we do pull the changes from the stable branches to run the CI so I would expect it to contain the changes for the release(I'll double-check to be sure). But like you said currently the build only runs when there are new changes through PR or push. Sometimes the changes in the PR might cause the CI to fail so some failures like this case might not be noticed until too late. Maybe we need to run nightly builds in release and maybe the master branch which will be more reliable in catching these kinds of errors early on. |
Fix incompatible constructor arguments for the activity classes by using the dependency injection container get method to automatically construct it.
Relevant upstream change nextcloud/activity#1309 but the adjustment here makes it less error prone for further changes.
Related WorkPackage:
This PR fixes: https://community.openproject.org/projects/nextcloud-integration/work_packages/50507/activity