Split procuration manager in five tabs. #876

hhamon · 2017-04-11T14:19:23Z

No description provided.

tgalopin · 2017-04-12T22:12:51Z

This probably need more work as we introduced filters in #881

hhamon · 2017-04-12T22:28:36Z

I'll finish this PR tomorrow morning ;)

HeahDude · 2017-04-13T07:59:34Z

src/AppBundle/Controller/ProcurationManagerController.php

-        $requestsRepository = $this->getDoctrine()->getRepository(ProcurationRequest::class);
-        $requests = $requestsRepository->findManagedBy($this->getUser(), (int) $page, self::PER_PAGE);
+        try {
+            $requests = $this->get('app.procuration.manager')->getProcurationRequests(strtolower($request->query->get('type', 'unprocessed')), self::PER_PAGE, (int) $page);


The int casting should be a parameter type hint in the signature (it will also cast).

HeahDude · 2017-04-13T08:00:11Z

src/AppBundle/Controller/ProcurationManagerController.php

@@ -157,29 +161,25 @@ public function requestAction(ProcurationRequest $request): Response
     * )
     * @Method("GET")
     */
-    public function requestTransformAction(ProcurationRequest $request, $action, $token): Response
+    public function requestTransformAction($id, $action, $token): Response


Please add parameters type hint.

HeahDude · 2017-04-13T08:01:43Z

src/AppBundle/Controller/ProcurationManagerController.php

    {
-        if (!$this->get('security.csrf.token_manager')->isTokenValid(new CsrfToken('request_action', $token))) {
+        if (!$this->isCsrfTokenValid('request_action', $token)) {


request_action should better be a constant.

it's only used here as far as I know

HeahDude · 2017-04-13T08:01:57Z

src/AppBundle/Procuration/ProcurationManager.php

+
+    private static function ensureProcessingType(string $type): void
+    {
+        if (!in_array($type, ['processed', 'unprocessed'])) {


Please use constants for types.

yep I thought about that.

All problems have been fixed

rpg600 · 2017-04-14T14:15:00Z

app/Resources/views/procuration_manager/_layout.html.twig

+                    --
+                    <a href="{{ path('app_procuration_manager_index', {'status': 'processed'}) }}">Mandants traités</a>
+                    --
+                    <a href="{{ path('app_procuration_manager_proposals') }}">Mandataires traités</a>


It's the same path for the processed and unprocessed ?

Oups I forgot to implement the status filtering for proxy proposals.

rpg600 · 2017-04-14T14:40:49Z

src/AppBundle/Procuration/Filter/ProcurationFilters.php

+            return;
+        }
+
+        if (!in_array($country, array_keys($this->getCountries()))) {


Missing third parameter ?

yes good catch.

rpg600 · 2017-04-14T14:43:37Z

src/AppBundle/Procuration/Filter/ProcurationProxyProposalFilters.php

+        parent::apply($qb, $alias);
+
+        $qb
+            ->addOrderBy('pp.createdAt', 'DESC')


maybe you can use $alias (if it contains 'pp') ?

I have to! Thanks!

rpg600 · 2017-04-14T14:59:18Z

src/AppBundle/Controller/ProcurationManagerController.php

+                'total_count' => $manager->countProcurationProxyProposals($this->getUser(), $filters),
+                'filters' => $filters,
+            ]);
+        } catch (ProcurationException $e) {


The whole try block is necessary ? (I know it's changes nothing)

Yes to convert ProcurationException exceptions into BadRequestHttpException exceptions.

I meant ProcurationException is only throw from the method fromQueryString right ?

rpg600 · 2017-04-14T15:00:51Z

src/AppBundle/Procuration/Filter/ProcurationRequestFilters.php

+
+    private $status = self::UNPROCESSED;
+
+    public static function fromQueryString(Request $request)


Missing return type hint

Impossible here. Because of the new static() in the parent class. We can't typehint against static.

mykiwi · 2017-04-15T17:11:47Z

app/Resources/views/procuration_manager/proposals.html.twig

-        {% endif %}
+    {% if 0 == proxies|length %}
+        <div class="text--body text--center">
+            {% if constant('AppBundle\\Procuration\\Filter\\ProcurationProxyProposalFilters::UNASSOCIATED') == filters.status %}


If the variable filters is an instance of ProcurationProxyProposalFilters you can use it to do constant('UNASSOCIATED', filters)

And all constant in this file.

Oh yes excellent thanks.

mykiwi · 2017-04-15T17:26:17Z

app/Resources/views/procuration_manager/requests.html.twig

+
+    {% if 0 == requests|length %}
+        <div class="text--body text--center">
+            {% if constant('AppBundle\\Procuration\\Filter\\ProcurationRequestFilters::PROCESSED') == filters.status %}


You can use filters here too I guess.

mykiwi · 2017-04-15T17:29:57Z

src/AppBundle/Procuration/Filter/ProcurationFilters.php

+        }
+
+        $qb
+            ->setFirstResult(($this->currentPage - 1) * $this->getLimit())


IMHO the method getLimit is useless. You should use the constant directly here.

hhamon · 2017-04-15T17:52:03Z

@mykiwi all requested changes have been applied ;)

… filters system.

HeahDude · 2017-04-19T10:22:41Z

src/AppBundle/Procuration/Filter/ProcurationFilters.php

+            throw new ProcurationException(sprintf('Invalid country filter value given ("%s").', $country));
+        }
+
+        $this->country = trim($country);


Why a second trim?

To be truly sure it's trimmed ^^
It must be removed for sure.

HeahDude · 2017-04-19T10:26:54Z

src/AppBundle/Procuration/Filter/ProcurationProxyProposalFilters.php

+    {
+        parent::apply($qb, $alias);
+
+        $status = $this->getStatus();


You should use a switch to evaluate only once without storing it in a variable.

I hate switch() statements, they make the code less readable IMO.

HeahDude · 2017-04-19T10:28:03Z

src/AppBundle/Procuration/Filter/ProcurationRequestFilters.php

+    {
+        parent::apply($qb, $alias);
+
+        if (self::UNPROCESSED === $this->getStatus()) {


Same here, it would be more readable with both cases explicit.

hhamon added Amélioration Work in progress labels Apr 11, 2017

hhamon self-assigned this Apr 11, 2017

hhamon force-pushed the feat-procuration-manager-split branch 7 times, most recently from 2890c88 to 33670e7 Compare April 11, 2017 15:37

HeahDude previously requested changes Apr 13, 2017

View reviewed changes

hhamon force-pushed the feat-procuration-manager-split branch 15 times, most recently from e1a45e9 to 98555e7 Compare April 14, 2017 14:02

hhamon added the Ready for code review label Apr 14, 2017

hhamon removed the Work in progress label Apr 14, 2017

hhamon requested review from mykiwi, rpg600 and tgalopin April 14, 2017 14:06

hhamon force-pushed the feat-procuration-manager-split branch 2 times, most recently from 94fb261 to 338ef45 Compare April 14, 2017 14:35

rpg600 reviewed Apr 14, 2017

View reviewed changes

hhamon force-pushed the feat-procuration-manager-split branch from 338ef45 to 9a3f8d1 Compare April 14, 2017 15:38

hhamon changed the title ~~Split procuration manager in 4 tabs.~~ Split procuration manager in five tabs. Apr 14, 2017

hhamon force-pushed the feat-procuration-manager-split branch 2 times, most recently from 8d25293 to ec7a9d3 Compare April 14, 2017 15:56

mykiwi suggested changes Apr 15, 2017

View reviewed changes

hhamon force-pushed the feat-procuration-manager-split branch from ec7a9d3 to e54f13e Compare April 15, 2017 17:51

hhamon force-pushed the feat-procuration-manager-split branch from e54f13e to 1ca33e3 Compare April 18, 2017 07:27

rpg600 approved these changes Apr 18, 2017

View reviewed changes

[AppBundle] split procuration manager in five tabs and refactored the…

af4909a

… filters system.

hhamon force-pushed the feat-procuration-manager-split branch from 1ca33e3 to af4909a Compare April 18, 2017 08:01

mykiwi approved these changes Apr 18, 2017

View reviewed changes

hhamon merged commit 75d1dab into master Apr 18, 2017

hhamon deleted the feat-procuration-manager-split branch April 18, 2017 09:36

HeahDude reviewed Apr 19, 2017

View reviewed changes


		private $status = self::UNPROCESSED;

		public static function fromQueryString(Request $request)

Split procuration manager in five tabs. #876

Split procuration manager in five tabs. #876

Conversation

hhamon commented Apr 11, 2017

tgalopin commented Apr 12, 2017

hhamon commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhamon Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykiwi Apr 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhamon commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhamon Apr 14, 2017 •

edited

Loading

mykiwi Apr 15, 2017 •

edited

Loading