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

[stable6] Backport of stable 6 for Nextcloud 26 #3431

Merged
merged 27 commits into from
Apr 30, 2024
Merged

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Apr 15, 2024

@come-nc come-nc self-assigned this Apr 15, 2024
@come-nc come-nc changed the base branch from master to stable-6 April 15, 2024 14:05
@dartcafe
Copy link
Collaborator

nice. Now I remember the incompatibility. 😁

@dartcafe
Copy link
Collaborator

I updated the branch protection for stable-6

@come-nc
Copy link
Contributor Author

come-nc commented Apr 16, 2024

nice. Now I remember the incompatibility. 😁

Do you think I missed anything? Do you know how complete are the unit tests?
I tested the UI a bit and it seems to work, but there is always the possibility of an incompatible signature in a corner-case code path.
Psalm seems happy so that’s good.

@dartcafe
Copy link
Collaborator

As soon as you are ready, add me as a reviewer. I will check then. Code wise it looks good.

@dartcafe
Copy link
Collaborator

I installed NC26 and will test some recent edge cases. But it may make sense to add the performance issues to the branch, so the stable-5 can die.

@come-nc come-nc marked this pull request as ready for review April 23, 2024 07:56
@come-nc come-nc requested a review from dartcafe April 23, 2024 07:56
artonge
artonge previously approved these changes Apr 24, 2024
.github/workflows/static-analysis.yml Show resolved Hide resolved
lib/Controller/VoteController.php Outdated Show resolved Hide resolved
lib/Controller/VoteController.php Outdated Show resolved Hide resolved
Saves a few DB requests

Signed-off-by: Côme Chilliet <[email protected]>
Avoid a DB request for each poll, instead use the data we just got from
 the full list.

Signed-off-by: Côme Chilliet <[email protected]>
Not pretty but avoids a few DB calls. Ideally we should not do these
 requests at all, I do not really understand this share part in Acl.

Signed-off-by: Côme Chilliet <[email protected]>
artonge
artonge previously approved these changes Apr 24, 2024
…ovements

Perf improvements for stable-6 for 26 branch
Not pretty but avoids a few DB calls. Ideally we should not do these
 requests at all, I do not really understand this share part in Acl.

Signed-off-by: Côme Chilliet <[email protected]>
artonge and others added 3 commits April 24, 2024 14:21
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge changed the title Backport of stable 6 for Nextcloud 26 [stable6] Backport of stable 6 for Nextcloud 26 Apr 24, 2024
artonge and others added 6 commits April 24, 2024 14:48
…ports

[stable6] Backports of perf improvements for stable 6 on 26
[stable6] fix: Add visual loading feedback for polls list
Signed-off-by: Maxence Lange <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
@dartcafe
Copy link
Collaborator

dartcafe commented Apr 26, 2024

I did some smoke testing and did not find any flaws so far. So OK from me!

Do you know how complete are the unit tests?

The whole testing is very poor and would need some significant coverage increase. See #3147 😉

dartcafe
dartcafe previously approved these changes Apr 26, 2024
@come-nc
Copy link
Contributor Author

come-nc commented Apr 29, 2024

@dartcafe dartcafe added this to the 6.2 milestone Apr 30, 2024
@dartcafe
Copy link
Collaborator

dartcafe commented Apr 30, 2024

@come-nc
Copy link
Contributor Author

come-nc commented Apr 30, 2024

  • Master is fine as it is, as well as the stable-6, or what do you have to do there?

You’re right master makes no sense, stable-6 is not compatible with Nextcloud master.
We could test with stable28 along with stable26 but I think testing the oldest supported version is enough for a stable version like this.

@come-nc
Copy link
Contributor Author

come-nc commented Apr 30, 2024

  • Please merge, whenever you are ready to (also to stable-6 ??)

I’m ready to merge, I just need your approval.

@dartcafe
Copy link
Collaborator

dartcafe commented Apr 30, 2024

Approval already given. Ah. Needed approval again. Done.
What do you think about merging #3467 first to this branch?
It will help to reduce the poll loading time by half.

@dartcafe dartcafe self-requested a review April 30, 2024 15:33
@come-nc come-nc merged commit bce814e into stable-6 Apr 30, 2024
27 checks passed
@delete-merged-branch delete-merged-branch bot deleted the stable-6-for-26 branch April 30, 2024 15:46
@come-nc
Copy link
Contributor Author

come-nc commented Apr 30, 2024

What do you think about merging #3467 first to this branch?
It will help to reduce the poll loading time by half.

I think it’s cleaner to merge to stable-6.
This branch was meant as a temporary effort to test if supporting 26 was possible, now that it’s merged we can merge other things to stable-6 to keep improvements going.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants