Pagination improvements #224
Conversation
…ve author specified).
…ts) and exercises pagination endpoint improved.
krulis-martin
force-pushed
the
pagination-improvements
branch
2 times, most recently
from
478d3bd to
51cc522
Compare
krulis-martin
force-pushed
the
pagination-improvements
branch
from
51cc522 to
86c19da
Compare
app/helpers/Pagination.php
Outdated
| /** | ||
| * Find appropriate MySQL Collation for given locale. | ||
| */ | ||
| public function getLocaleCollation() |
There was a problem hiding this comment.
I am not really sure, if this is a good place for this function...
There was a problem hiding this comment.
I am not happy about it either. But there are very few options. Only other two choices would be to embed it in OrderByCollationInjectionMysqlWalker (which is slightly worse in my opinion) or place it in a separate helper (which is overkill in case of such a tiny function). If you have other alternatives in mind which I have overlooked, feel free to suggest them... :)
There was a problem hiding this comment.
For me, the better place would be the OrderByCollationInjectionMysqlWalker, but I do not insist on it, if you think it would be worse...
| ], $code); | ||
| } | ||
| /* | ||
| protected function getPagination(int $offset, ?int $limit, string $orderBy = null, array $filters = []): Pagination { |
app/model/repository/Users.php
Outdated
|
|
||
| $expr = $qb->expr()->orX(); | ||
| foreach (["u.firstName", "u.lastName"] as $column) { | ||
| $expr->add($qb->expr()->like($column, $qb->expr()->literal('%' . $search . '%'))); |
There was a problem hiding this comment.
search parameter is not splitted, this causes that queries like Martin Polanka do not find anything.
app/helpers/Pagination.php
Outdated
| if (!$collations) { | ||
| // First time initialization ... | ||
| $collations = [ | ||
| 'cs' => 'utf8_czech_ci' |
There was a problem hiding this comment.
What about en collation? Do you assume that it is default and therefore you do not have to set it?
There was a problem hiding this comment.
Exactly. But I can add it, if you would like.
| return $this->pipelineAcl->canViewDetail($pipeline); | ||
| }); | ||
| $this->sendPaginationSuccessResponse($pipelines, $pagination, []); | ||
| $this->sendPaginationSuccessResponse($pipelines, $pagination, true); // true = slice manually here |
There was a problem hiding this comment.
more like automatically... no? :-)
There was a problem hiding this comment.
My thinking was: automatically = already done by database, otherwise we need to do it manually in PHP code. Maybe the most clear description would be true = sliced by pagination class
There was a problem hiding this comment.
yes, it would definitely be more clear ;-)
app/model/repository/Pipelines.php
Outdated
| } | ||
|
|
||
| // Add order by | ||
| if ($pagination->getOrderBy() && !empty(self::$knownOrderBy[$pagination->getOrderBy()])) { |
There was a problem hiding this comment.
If I am not mistaken, this piece of code is the same for Users, maybe a helper function would be on point? ... the same goes for detection of search filter?
There was a problem hiding this comment.
OK. I will give it a serious thought.
| "totalCount" => count($items), | ||
| "items" => $sliceItems | ||
| ? array_slice(array_values($items), $pagination->getOffset(), | ||
| $pagination->getLimit() ? $pagination->getLimit() : null) |
There was a problem hiding this comment.
This should omit the zero? Or why is this ternary here?
There was a problem hiding this comment.
Zero limit has "give me everything" semantics.
| } | ||
|
|
||
| return $this->searchHelper($search, function ($search) { | ||
| $idsQueryBuilder = $this->em->createQueryBuilder()->addSelect("l.id")->from(LocalizedExercise::class, "l"); |
There was a problem hiding this comment.
Is the searchByName function used somewhere else than in the Exercises:default action? Or why it stayed here?
There was a problem hiding this comment.
It is definitely used in tests since it is a convenient way to get a user. I have decided to keep it rather than fixing all the test (plus using pagination just to fetch one user by the name is a bit tedious).
| * @param string $instanceId Id of an instance from which the authors are listed. | ||
| * @param string|null $groupId A group where the relevant exercises can be seen (assigned). | ||
| */ | ||
| public function actionAuthors(string $instanceId = null, string $groupId = null) |
There was a problem hiding this comment.
What is a purpose of this endpoint? And why don't you use mass endpoint to obtain users based on IDs?
There was a problem hiding this comment.
Purpose is to offer authors for selection in pagination filter (since there are too many users, even supervisors, to offer them all). I thought about passing on the user IDs only, but since full user entities are required in all UI scenarios and since the database selects all the users anyway, it is more efficient and practical to pass the users as complete entities.
tests/base/PresenterTestHelper.php
Outdated
| * @throws Tester\AssertException | ||
| * @throws JsonException | ||
| */ | ||
| public static function preformPresenterRequest($presenter, string $module, string $method = 'GET', array $params = [], $expectedCode = 200) |
… to a helper class. Improving search allowing multiple search tokens to be passed.
No description provided.