fixed plugin search issue under pagination section

[somya51p]

@jennydaman Please review the PR and suggest changes if any. It resolved issue #246 from my end.

[jennydaman]

Hey @somya51p sorry for not noticing your comment on issue #246.

Could you explain to me why this bug is happening? Your fix seems like a band-aid solution.

[somya51p]

Hey @somya51p sorry for not noticing your comment on issue #246.

Could you explain to me why this bug is happening? Your fix seems like a band-aid solution.

@jennydaman For the pagination offset it was showing 1 to -1, which is irrelevant, so I have just tried to replace it according to the plugins.totalCount as totalCount = -1 is coming from the backend side as discussed in issue #246 earlier.

[jennydaman]

Ah, thanks @somya51p, Please forgive my bad memory.

I do remember this discussion, I'm not sure but I think I might've said somewhere that it'd be better to fix this at the level of the library @fnndsc/chrisstoreapi? Or perhaps at a higher level, with an explanation? Ideally we would like to separate application logic from display logic.

I also see this line of code which should probably be used:

ChRIS_store_ui/src/components/Plugins/Plugins.jsx

Line 371 in e9eb46d

const pluginsCount=plugins.totalCount > 0 ? plugins.totalCount : 0;

[somya51p]

@jennydaman I get your point, in case of no result it should obviously show something like -

and would this code be satisfactory for the above case? -

[jennydaman]

I don't understand the context of your screenshot, would you generate a patch instead? To do so, you can run

git diff > changes.patch

Then paste the contents of changes.patch here.

[somya51p]

I don't understand the context of your screenshot, would you generate a patch instead? To do so, you can run

git diff > changes.patch

Then paste the contents of changes.patch here.

@jennydaman Please have a look!

diff --git a/src/components/Plugins/Plugins.jsx b/src/components/Plugins/Plugins.jsx
index 1748db8..51047d2 100644
--- a/src/components/Plugins/Plugins.jsx
+++ b/src/components/Plugins/Plugins.jsx
@@ -416,11 +416,11 @@ export class Plugins extends Component {
                             <p style={{ fontSize: '1.25em', margin: '0', color: 'black', fontWeight: '600' }}>
                                 {pluginsCount} plugins found
                             </p>
-                            Showing {paginationOffset + 1} to {' '}
+                            Showing {Math.min(pluginsCount, paginationOffset + 1)} to {' '}
                             {
                               // eslint-disable-next-line no-nested-ternary
-                              (paginationOffset + paginationLimit > plugins.totalCount) ?
-                                plugins.totalCount
+                              (paginationOffset + paginationLimit > pluginsCount) ?
+                                pluginsCount
                                 :
                                 (paginationOffset > 0) ?
                                   paginationOffset

[jennydaman]

@somya51p thank you for the patch, it helped me understand.

Showing {Math.min(pluginsCount, paginationOffset + 1)} to {' '}

I don't think this would work if you were on the second or third page of plugins.

What I meant was to move the logic outside of the return statement. As you see, there's a lint disabling comment // eslint-disable-next-line no-nested-ternary, indicating that a messy code pattern was used. I think the code quality could be improved by moving these outside the return statement and adding comments for why its necessary,

diff --git a/src/components/Plugins/Plugins.jsx b/src/components/Plugins/Plugins.jsx
index ad318e6..f52850f 100644
--- a/src/components/Plugins/Plugins.jsx
+++ b/src/components/Plugins/Plugins.jsx
@@ -368,8 +368,11 @@ export class Plugins extends Component {
       </div>
     )
 
+    // workaround for library antipattern where, if a search returns no results,
+    // the literal `-1` is returned.
+    // https://github.com/FNNDSC/fnndsc/blob/c5db17c42aef5222faaff08affbeb418234c3af9/js/chrisStoreAPI/src/resource.js#L364
     const pluginsCount=plugins.totalCount > 0 ? plugins.totalCount : 0;
-    
+
 
     return (
       <article>
@@ -420,7 +423,7 @@ export class Plugins extends Component {
                             {
                               // eslint-disable-next-line no-nested-ternary
                               (paginationOffset + paginationLimit > plugins.totalCount) ?
-                                (plugins.totalCount === -1 ? 1 : plugins.totalCount)
+                                (pluginsCount)
                                 :
                                 (paginationOffset > 0) ?
                                   paginationOffset

[somya51p]

@jennydaman Please review, I have tested for the plugins it is working and added these outside the return statement for code quality improvement.

For lint disabling comment // eslint-disable-next-line no-nested-ternary
we can implement that way, but I don't think it's as good a way.

// no-nested-ternary rule disallows nested ternary expressions
    const pluginsLast=(paginationOffset + paginationLimit > pluginsCount) ?
    pluginsCount : paginationLimit;
    if (paginationOffset + paginationLimit > pluginsCount)
          pluginsLast = pluginsCount;
    else if (paginationOffset > 0)
          pluginsLast = paginationOffset;
    else
          pluginsLast = paginationLimit;
    

[jennydaman]

The only situation where pluginCount < paginationOffset + 1 would be where pluginCount == 0, in which case, it might be better to instead conditionally render something like "No plugins shown" or just omit this text instead. If we can get rid of this Math.min call, the what and why of this line of code would be more apparent by a quick glance.

[somya51p]

@jennydaman I have tried to implement the logic which you have suggested. Please have a look into it and suggest changes if any.

[jennydaman]

We should fix this in the upstream client library FNNDSC/fnndsc#76