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(authServ): repair auth server page in BO #34106
Conversation
tleon
commented
Sep 29, 2023
•
edited by jolelievre
edited by jolelievre
Questions | Answers |
---|---|
Branch? | develop |
Description? | A field in the ApiAccess entity has been renamed in another PR. The experimental BO page was broken because of this change. This PR fixes this. |
Type? | bug fix |
Category? | BO |
BC breaks? | no |
Deprecations? | no |
How to test? | Activate the experimental Auth Server page. Then access it. |
UI Tests | https://github.com/tleon/ga.tests.ui.pr/actions/runs/6353483501 |
Related PRs | #33833 |
Sponsor company | PrestaShop SA |
Hi, thanks for this contribution! I found some issues with the Pull Request description:
Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much! About linked issuesPlease consider opening an issue before submitting a Pull Request:
(Note: this is an automated message, but answering it will reach a real human) |
f0a53be
228b28f
to
f0a53be
Compare
f0a53be
to
ace7709
Compare
c386cce
to
5e9be28
Compare
Hello @tleon Thank you for your PR, I tried it and unfortunately, I can't go on auth serv when I install your PR as you can see : Untitled_.Oct.2.2023.3_07.PM.webmWaiting for your feedback |
5e9be28
to
32d1d1e
Compare
@@ -108,10 +108,9 @@ private function applyAssociatedQueries(QueryBuilder $queryBuilder): void | |||
private function appendActiveApiAccessQuery(QueryBuilder $queryBuilder): void | |||
{ | |||
$shopQueryBuilder = $this->getQueryBuilder() | |||
->select('count(api.active)') | |||
->select('count(api.enabled)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
->select('count(api.enabled)') | |
->select('count(api.id_api_access)') |
Seems like it makes more sense to test the count of IDs no? Like in the other method
->from($this->dbPrefix . 'api_access', 'api') | ||
->where('api.id_authorized_application = aa.id_authorized_application') | ||
->andWhere('api.active = 1'); | ||
->Where('api.enabled = 1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed this condition because the relationship has been removed in the ApiAccess entity right? This changes the behavior of the grid, but it's supposed to be completely removed soon anyway so as long as tests do not shout I guess we don't really care
Closed in favor of #34146 |