Loris icon indicating copy to clipboard operation
Loris copied to clipboard

Improve view session

Open maximemulder opened this issue 2 years ago • 3 comments

Brief summary of changes

As the imaging browser was in my testing assignments and I need to learn more about imaging, I took this opportunity to improve the view session page. The module is not fully refactored, but it is certainly cleaner than before imo.

I modernized the front-end by porting some parts of the interface to React / Typescript and improving existing code. I modified the back-end to this effect by splitting template data creation into several JSON API endpoints. This back-end modification is however not really an improvement (but not worse either imo) because I do not have enough knowledge yet about our achitecture and database to do so.

Testing instructions (if applicable)

  1. Go to imaging browser / view session
  2. Compare behaviour and interface to demo

Link(s) to related issue(s)

  • Resolves #8445
  • Partially resolves #9008

maximemulder avatar Feb 07 '24 20:02 maximemulder

I did not comply to all the lints yet because I think some would decrease the quality of the code. So I'll discuss that before doing anything else.

It is not me who is wrong it's the system !!!

maximemulder avatar Feb 07 '24 20:02 maximemulder

I would strongly suggest breaking this up into smaller pieces. This is effectively rewriting the imaging browser and the people who know the module best are likely to take a long time to review/testing a change of this size. Smaller pull requests are, generally, much easier to review and test. A natural divider for those pull requests might be something like porting the components from JS to TS one component at a time.

Just skimming it, some things that I noticed which would come up on those smaller PRs:

  1. The LORIS code uses PSR7, not PHP superglobals for accessing query parameters. If you need something somewhere that the object isn't available (such as hasAccess) the loadResources function is called early in the request and information can be extracted and stored in a class variable
  2. I noticed there are some React.createFactory() lines. That is a very old style which is deprecated (https://react.dev/reference/react/createFactory). I believe they only ever existed to access React components from Smarty in the early days of React in LORIS. Components are generally imported from js/ts now (I didn't look closely enough to see if that's the case here.)

driusan avatar Feb 08 '24 15:02 driusan

Sorry for the wait, I agree there is a lot of code to review. However, it is mostly a "dumb" port of the module front-end to a more modern stack so I think it can be reviewed somewhat nicely provided the following review guide.

So, this PR is a partial port of the view session front-end to the modern Typescript/React stack with an adaptation of the back-end to provide the appropriate module API calls.

Basically, the jsx/imagePanel.js file has been modernized and split into the jsx/SessionImage*.tsx files. Those are simple typed React components so the code should be easily understandable imo. There are a few style improvements compared to the old interface, but nothing very big (mostly moving the image headers button, better alignments of the image buttons, and directly display the image panels on the page instead of in a parent panel).

On the back-end, the three files getimagedata.class.inc, getsubjectdata.class.inc and getscannerdata.class.inc provide endpoints to feed data to the front-end, this was previously done with templates. Their code is not original and was basically extracted from viewsession.class.inc. There are 2~3 duplicated methods (notably _hasAccess()) but it should not be too problematic imo. I insist that I did not introduce new logic on the back-end and this is mostly just "moving code".

Now for the auto-review, I believe this PR is a straightforward improvement of the module, but there is still a lot to improve in the future:

  1. This PR is only a partial refactor of the module. I did it because I had the time to do so, but it is not yet perfect. This is notably why there is a React.createFacory as Smarty templates are still used, although strictly less than before (it was already present before).

  2. This PR lacks validation on the query parameters for the module API calls. As far as I have seen this is not problematic, except for the response being full of nulls in case of an incorrect parameter. From what I have seen, Loris lacks this validation in many places. However, if you want me to add this validation, just point me to some existing Loris code that does it and I will happily replicate it.

  3. the API endpoints do not use modern data providers. The reason for that is that the current back-end code does not translate well to the "single database query" architecture.

Anyway, I've given all the important information I could think of. If anyone wants to talk about the PR vocally, just tell me here or on Slack. I will be pretty busy the coming weeks, but if I ever get the time to do so, I'd like to write another PR to at least finish the modernization of the module's front-end and remove the remaining templates.

maximemulder avatar Feb 13 '24 15:02 maximemulder