[OMP] Only one featured monograph is displayed

[alexdryden]

Version: OMP3.2.0-3

Issue: Featured monographs share the same order index (1) by default unless they are explicitly reordered by the user. As a result only one featured monograph is displayed.

Steps to reproduce:

1. Start with a few books and make none of them featured.
2. Select checkboxes to make them all featured.
3. Select "View Site" and only one title is listed as featured

Here is a diff result for the dbdump before and after explicitly ordering. First one (<) is before, second one (>) is after. The last value for each entry is the the order index, seq.

< INSERT INTO `features` VALUES (3,512,2,1),(4,512,2,1),(5,512,2,1);
---
> INSERT INTO `features` VALUES (3,512,2,2),(4,512,2,1),(5,512,2,0);

[NateWr]

Thanks @alexdryden. This sounds like maybe a bug. Are you able to look into it further? It's probably going to be an issue with where the featured items are saved. I think that's an op like saveFeatureOrder or something like that.

[alexdryden]

@NateWr I looked into it a little bit last week. I'm still not super familiar with the architecture, so this may not be the best solution, but it seemed like the best place to patch this would be in the insertFeature() function in the FeatureDAO class here by checking for the MAX value in the seq column and setting $seq to max value + 1.

The function is asking for the $seq value explicitly, but when this is used to simply add another publication to the list of features the default behavior should be to push it to the end of the stack of featured submissions. Right now it is just getting a $seq value of 1 in those cases. Here is my take on a solution in that function.

That solution doesn't break the saveFeaturedOrder() function, which calls insertFeature(), but instead of explicitly getting $seq passed from the form it gets it implicitly from the order of features in in the form. I'll keep looking at it and see if there is a better solution that doesn't rely on implicit ordering.

[NateWr]

Yeah that looks like it would work, but we try to keep that kind of business logic out of the DAOs. Ideally they should just be for reading and writing data, and the logic which determines what the data looks like should be handled elsewhere.

It looks like the problem is in CatalogListItem.vue and it applies to features and new releases. Unfortunately, the proper seq to assign is not available at that level.

I think there are two options here:

1. The code in CatalogListItem.vue that toggles featured and new release assignments and saves that data through the API could be moved to CatalogListPanel.vue. Then CatalogListItem.vue can emit an event that triggers that to happen in CatalogListPanel.vue, where the appropriate seq could be determined and assigned. This strikes me as the most appropriate route, but it requires some familiarity with Vue.js and a fair amount of refactoring on the frontend.

2. Another option would be to explicitly not set a seq in CatalogListItem.vue, or to set it to something like -1. This could then be detected when the request is handled by the API (here). And something like the code you proposed, which got the max sequence and added it in, could be applied before insertFeature() is called.

[alexdryden]

Yeah, something like option 2 was where I was heading. I'll check out option 1 first, if for no other reason then to help familiarize myself with more of the code base, and then fall back to option 2 if it seems like more work than I can fit in this week.

[mfelczak]

Just adding a +1 here. This bug was recently reported from a hosted OMP install running 3.2.1-1

[alexdryden]

I was working on this right around when my partner got covid and it totally fell off my radar. I can submit a pull request when I'm back in the office next week for option 2 listed above, which is I think what I implemented for us.

[iamtiagodev]

This problem still happens. It's not my intention to propose a proper fix but I think I found the culprit:

SELECT submission_id, seq FROM features WHERE assoc_id = 3 ORDER BY seq;
+---------------+----------+
| submission_id | seq |
+---------------+----------+
|            10 |   1  |
|            11 |   1  |
+---------------+----------+

When the featured checkbox is ticked, it will add a new entry to features table with default sequence value as 1.
If a sequence value is duplicated it will produce unexpected results uppon using array_flip function here

I think the proper fix should be calculating sequence value based on the last stored value upon creating it.

[NateWr]

Good hunting, @duozhasht! We've proposed a couple of possible approaches to getting the correct value stored in this comment. I think the second approach is probably the better.

We're open to a PR from the community on this, or we'll try to get to it as part of our next major release cycle.

[thiagolepidus]

A similar problem is occurring when saving the features order, limiting the number of featured monographs to 30. This occurs because only items present on the current page of the "Oder Features" tab are saved.

Mysql query before save order:

SELECT COUNT(*) FROM features;
+----------+
| COUNT(*) |
+----------+
|       50 |
+----------+
1 row in set (0,00 sec)

mysql query after save order:

SELECT COUNT(*) FROM features;
+----------+
| COUNT(*) |
+----------+
|       30 |
+----------+
1 row in set (0,00 sec)

We have created an issue about this: #7648

[kaitlinnewson]

PRs:

• main: pkp/pkp-lib#6088 set sequence for featured monographs omp#1723
• 3.4: pkp/pkp-lib#6088 set sequence for featured monographs omp#1736
• 3.3: pkp/pkp-lib#6088 set sequence for featured monographs omp#1737

@bozana are you available to review this? I followed a similar approach to what is used for resequence() in ContextDAO.

I also fixed a small CSRF issue I encountered (related to migration work done in #9566).

I targeted this against main due to the issue's milestone, but can back-port once approved.

[bozana]

All well and merged to mein @kaitlinnewson. Thanks a lot!

[kaitlinnewson]

@bozana Backported and the new PRs are listed above.

[bozana]

Thanks @kaitlinnewson!
All merged, thus closing...