Make Surf representative a role on its own #638
Conversation
MKodde
force-pushed
the
feature/surfconext-verantwoordelijke-authz
branch
4 times, most recently
from
116d0cf
to
992153c
Compare
MKodde
force-pushed
the
feature/surfconext-verantwoordelijke-authz
branch
from
992153c
to
51a098e
Compare
MKodde
force-pushed
the
feature/surfconext-verantwoordelijke-authz
branch
from
51a098e
to
c9d7f20
Compare
| $ignoreErrors[] = [ | ||
| 'message' => '#^Parameter \\#1 \\$entityName of class Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\ViewObject\\\\EntityConnection constructor expects string, string\\|null given\\.$#', | ||
| 'count' => 2, | ||
| 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Service/ServiceConnectionService.php', | ||
| ]; |
There was a problem hiding this comment.
Simple to fix with a ? in front of the variable?
There was a problem hiding this comment.
Yep, that could be a solution. But I do not want to loosen type safety, the creating side should be more defensive. And thats where this baseline entry came from.
| $this->getTestIdpsIndexed(), | ||
| $service, |
There was a problem hiding this comment.
You could consider using named parameters, then the order doesn't matter
There was a problem hiding this comment.
I never considered using that. in this case the service argument became optional, and needed to be moved to the bottom of the stack.
Leaving this as-is. But thanks for the suggestion for using the NamedParameters. Going to read up on them.
| class EntrypointController extends AbstractController | ||
| { | ||
| #[Route(path: '/', name: 'entrypoint', methods: ['GET'])] | ||
| public function overview(): RedirectResponse |
There was a problem hiding this comment.
Love these simple controllers... The only change I would make is to use __invoke() instead
There was a problem hiding this comment.
Ah, yes, another SF construction I need to get used to. Changing that
| $service | ||
| ); | ||
| } | ||
| if ($serviceCount === 0 && empty($allowedServices)) { |
There was a problem hiding this comment.
What does it mean in one word when serviceCount === 0 and allowedservices is empty (maybe use [] instead)
If it can be one word, then use a local method that explains this, because I do not see what is thi state represents
There was a problem hiding this comment.
Top feedback. I have replaced the expression with a boolean variable that suggests the state being tested.
| @@ -88,6 +88,9 @@ class WebTestCase extends PantherTestCase | |||
|
|
|||
| protected DevelopmentIssueRepository $jiraIssueRepository; | |||
|
|
|||
| private string $surfConextRepresentativeAttributeName = ''; | |||
| private string $surfConextRepresentativeAttributeValue = ''; | |||
|
|
|||
| public static function setUpBeforeClass(): void | |||
| { | |||
| exec('cd /var/www/html && composer dump-env test -q && chmod 777 /tmp/spdashboard-webtests.sqlite'); | |||
There was a problem hiding this comment.
This we can do better by using sqlite in memory, I have an example setup in my private repo how to do this. Not for now, but later on. Basically I used this https://www.sitepoint.com/quick-tip-testing-symfony-apps-with-a-disposable-database/
There was a problem hiding this comment.
In this case, for the simple fixtures, I find it more flexible to simply store json blobs in txt files. I am interested in your thoughts about optimising our set up. Which is really basic, and bare bones at this point.
Also be aware that our SQL persistence layer is wafer thin, we store all the things in OpenConext Manage. The SQLite test substitute is a powerfull test option, but in this application of limited use.
Instead we opted to run our containers in test mode. Automatically switching to a set of test databases. (like we do in the stepup tests)
MKodde
force-pushed
the
feature/surfconext-verantwoordelijke-authz
branch
from
c9d7f20
to
8c12aae
Compare
Previously it inherited the default ROLE_USER role. Which also granted access to the /entity and /services pages. That is not required for now. The representative should only see the /connections page.
The missing web test coverage was added in this commit. Some notes: - For now all IdPs are allowed for every SP (via allowedall) - Only one test idp is configured, that could be expanded upon
MKodde
force-pushed
the
feature/surfconext-verantwoordelijke-authz
branch
from
8c12aae
to
e66a0ff
Compare
Previously it inherited the default ROLE_USER role. Which also granted access to the /entity and /services pages. That is not required for now. The representative should only see the /connections page.
Disclaimer: The new addition to the phpstan baseline was 'required' as a more rigorous refactoring was required to fix the issue. And that was out of scope at this point.