pkp/pkp-lib#1660 Customizable Reviewer Recommendations #444

touhidurabir · 2024-11-06T11:17:55Z

asmecher · 2025-02-28T00:20:11Z

src/components/ListPanel/reviewerRecommendations/ReviewerRecommendationsListPanel.vue

+			this.openDialog({
+				name: 'delete',
+				title: this.deleteRecommendationLabel,
+				message: this.replaceLocaleParams(this.confirmDeleteMessage, {


This is an XSS flaw -- any HTML entered in the recommendation.title will be passed through to the browser. (Also the other keys that mix user-supplied data in here.)

@asmecher good spot . I had this left out as v-strip-unsafe-html was not yet available for main as we introduced in 3.4.0 . But then forgot to update these as v-strip-unsafe-html was added just about a week ago in main branch .

One question , rather than applying this for replaceLocaleParams every time like

message: this.replaceLocaleParams(this.confirmDeleteMessage, { title: sanitizeHtml(this.localize(recommendation.title)), })

should we add this sanitizeHtml into the implementation of replaceLocaleParams so that we don't need to make sure to add this every time ?

XSS is by default prevented by vue.js, and in cases where we use (and sometime overuse) v-html, its now replaced with v-strip-unsafe-html. Therefore this will go through that. Every component is responsible to do the XSS protection.

Therefore Dialog handles that when rendering message.

<div v-strip-unsafe-html="message" />

No point to sanitize twice.

@jardakotesovec, I used a <i> tag to confirm that entered HTML was passed through, and I didn't check whether more malicious tags were filtered -- so it's possible that <i> gets through because it's "safe", but worse HTML would be filtered. However, it's still incorrectly escaping the content. The reviewer recommendation title is plaintext, not a rich text field, so any entered content should be fully escaped before getting glued into the locale string.

jardakotesovec · 2025-02-28T12:15:50Z

src/components/ListPanel/reviewerRecommendations/ReviewerRecommendationsListPanel.vue

+import dialog from '@/mixins/dialog.js';
+import fetch from '@/mixins/fetch';
+import cloneDeep from 'clone-deep';
+import ReviewerRecommendationsEditModal from './ReviewerRecommendationsEditModal.vue';


I see that you created some components with options api and some with composition api. This looks bit of mix of both. Mainly options API, but also using composables which are mainly intended for composition API.

Any chance we could have this consistently using composition api.

There is no need to use ajaxError, dialog, fetch mixins. We have all these covered in useModal and useFetch. We already have lots of usage examples and these are actually quite well documented and I am also able to respond on mattermost for any usage questions.

jardakotesovec · 2025-02-28T12:35:28Z

src/components/ListPanel/reviewerRecommendations/ReviewerRecommendationsListPanel.vue

@@ -0,0 +1,472 @@
+<template>


I am putting these new 'managing' components to the src/managers

We could name it something like ReviewerRecommendationsManager

jardakotesovec · 2025-02-28T12:38:36Z

src/components/ListPanel/reviewerRecommendations/ReviewerRecommendationsListPanel.vue

@@ -0,0 +1,472 @@
+<template>
+	<div data-cy="reviewer-recommendation-list-panel">
+		<PkpTable aria-label="Reviewer recommendations list">


You can find more details in storybook - https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/components-table--docs#general-guidelines

But long story short, aria-label is unnecessary. This table gets automatically linked to its label, which you did provide. So that fine. And even if would use aria-label somewhere it would have to be localised.

jardakotesovec · 2025-02-28T12:43:41Z

src/components/ListPanel/reviewerRecommendations/ReviewerRecommendationsListPanel.vue

+					<TableCell>
+						<label>
+							<input
+								class="text-blue-900 bg-gray-100 border-gray-300 focus:ring-blue-500 dark:focus:ring-blue-600 dark:ring-offset-gray-800 dark:bg-gray-700 dark:border-gray-600 h-5 w-5 rounded focus:ring-2"


Styled checkboxes probably come with redesigned forms.

So yeah, we don't have that yet. It would be fine to use unstyled checkbox meanwhile. But not big deal either way.

jardakotesovec · 2025-02-28T12:48:34Z

src/pages/dashboard/composables/useDashboardConfigReviewActivity.js

-			return RecommendationTranslations[reviewAssignment.recommendation]
-				? t(RecommendationTranslations[reviewAssignment.recommendation])
-				: null;
+			const recommendation = recommendations.filter(


for this use case I think recommendations.find would be better fit?

jardakotesovec · 2025-02-28T12:51:10Z

src/pages/dashboard/composables/useDashboardConfigReviewActivity.js

@@ -277,7 +276,7 @@ function getDays(config, reviewAssignment) {
 	return null;
 }

-export function useDashboardConfigReviewActivity() {
+export function useDashboardConfigReviewActivity(recommendations) {


very minor thing - I am trying to follow convention of using one object as first arguments, even if its one thing, because in future there might be more. So it could be useDashboardConfigReviewActivity({recommendations})

jardakotesovec

Thanks @touhidurabir. This is very nicely integrated with new code base. I am not sure whether this is aimed for 3.5, so I would understand if some of the 'composition api' updates would be left out for timeline purposes.

But if you decide to do the upgrade and struggle with something feel free to message me directly on mattermost. I should be able to point you to some good example or something.

asmecher · 2025-02-28T14:58:43Z

I am not sure whether this is aimed for 3.5

It's scheduled for 3.6, and I was considering pulling it forward to 3.5 -- but I'd rather spend a bit more time to get a clean implementation than rush it in. So let's leave it on the 3.6 milestone.

…mmendations pulling

…omponent to support OMP/OPS

touhidurabir mentioned this pull request Nov 6, 2024

Customizable Reviewer Recommendations pkp/pkp-lib#1660

Open

1 task

touhidurabir force-pushed the i1660_main branch 2 times, most recently from 73e1b28 to 664f865 Compare November 19, 2024 07:46

touhidurabir force-pushed the i1660_main branch 2 times, most recently from 6ffce94 to 843402d Compare December 6, 2024 11:34

touhidurabir force-pushed the i1660_main branch from a6bffdd to eb21d67 Compare December 21, 2024 08:57

touhidurabir force-pushed the i1660_main branch 2 times, most recently from 6a02187 to 3a9b03d Compare January 10, 2025 12:05

touhidurabir force-pushed the i1660_main branch from 3a9b03d to 9a47b49 Compare January 23, 2025 05:40

touhidurabir requested a review from asmecher January 24, 2025 17:43

touhidurabir force-pushed the i1660_main branch from 9a47b49 to fe6774b Compare January 26, 2025 06:23

touhidurabir force-pushed the i1660_main branch 2 times, most recently from 406d064 to 253ac65 Compare February 11, 2025 09:08

touhidurabir force-pushed the i1660_main branch 2 times, most recently from 70c9f3f to b8beae3 Compare February 23, 2025 17:03

touhidurabir force-pushed the i1660_main branch 2 times, most recently from 3fc09e1 to a3dc39d Compare February 27, 2025 19:13

touhidurabir marked this pull request as ready for review February 27, 2025 20:06

asmecher requested changes Feb 28, 2025

View reviewed changes

jardakotesovec reviewed Feb 28, 2025

View reviewed changes

jardakotesovec requested changes Feb 28, 2025

View reviewed changes

touhidurabir added 3 commits March 3, 2025 17:03

pkp/pkp-lib#1660 Customizable Reviewer Recommendations

4eb9867

pkp/pkp-lib#1660 remove static recommendations and added dymanic reco…

5626b0a

…mmendations pulling

pkp/pkp-lib#1660 updated activate/deactivate process

38495a4

touhidurabir added 7 commits March 3, 2025 17:04

pkp/pkp-lib#1660 behaviour update of checkbox to activate/deactivate

6729ed6

pkp/pkp-lib#1660 table view implementation

6d0518f

pkp/pkp-lib#1660 set used recommendation uneditable

0b85ebb

pkp/pkp-lib#1660 updated dropdown action component

2f2de43

pkp/pkp-lib#1660 removed unused imports

99f6ea7

pkp/pkp-lib#1660 make recommendations optional for reviewer manager c…

d5796bb

…omponent to support OMP/OPS

pkp/pkp-lib#1660 added js side html sanitizer to handle XXS issue

b5e83bf

touhidurabir force-pushed the i1660_main branch from e37bdc2 to b5e83bf Compare March 3, 2025 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#1660 Customizable Reviewer Recommendations #444

pkp/pkp-lib#1660 Customizable Reviewer Recommendations #444

touhidurabir commented Nov 6, 2024

asmecher Feb 28, 2025

touhidurabir Feb 28, 2025 •

edited

Loading

jardakotesovec Feb 28, 2025

asmecher Feb 28, 2025

jardakotesovec Feb 28, 2025

jardakotesovec Feb 28, 2025

jardakotesovec Feb 28, 2025

jardakotesovec Feb 28, 2025

jardakotesovec Feb 28, 2025

jardakotesovec Feb 28, 2025

jardakotesovec left a comment

asmecher commented Feb 28, 2025

pkp/pkp-lib#1660 Customizable Reviewer Recommendations #444

Are you sure you want to change the base?

pkp/pkp-lib#1660 Customizable Reviewer Recommendations #444

Conversation

touhidurabir commented Nov 6, 2024

Choose a reason for hiding this comment

touhidurabir Feb 28, 2025 • 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

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

jardakotesovec left a comment

Choose a reason for hiding this comment

asmecher commented Feb 28, 2025

touhidurabir Feb 28, 2025 •

edited

Loading