[bvl_feedback] Feedback summary and thread list only viewable with access_all_profiles permission

[jesscall]

Issue originally reported by @racostas on CCNA Repo during candidate release testing of LORIS 23.0.2
aces/CCNA#4269

---

Not consistent behaviour of bvl_feedback at the profile level when the user has the permission Behavioural QC but not the Across all sites access candidate profiles one.

How to test:
(take a user having the permission Behavioural QC but not the Across all sites access candidate profiles (neither the superadmin one))

1. Go to MainMenu->Canditate-> Access Profile.
2. Click on a candidate of choice.
3. Open the bvl_feedback panel clicking on the notepad icon in the left part of the Main Menu bar (at the left of the help button sign)
4. Create a new feedback on the New profile level feedback tab (middle one)

5. Notice in this same middle panel the message(The new thread has been submitted!) but the upper Feedback Threads and bottom Feedback Threads tabs remains unchanged . (if the same process is done with a superuser account or a user having at the time Behavioural QC but and Across all sites access candidate profiles these two tabs are populated. [Please note that the not shown info is nevertheless stored]

6. In the dashboard (clicking in the LORIS button at the top left) the feedback does shows on the Behavioural Feedback Notifications (the same not shown in step 5)

7. If the notification referred in step 6 is clicked, and then again in the bvl_feedback button (the notepad icon) the feedback is not shown (step 5).

NOTE: Please note that this issue is not observed when in the same conditions the user is providing the feedback at the Visit or Instrument level

[jesscall]

TL;DR: Cannot view a (profile) feedback at the profile level if you do not have across all sites permission.

I don't think this is the intended behaviour. Will issue fix.

@driusan @ridz1208 fyi

[jesscall]

@driusan @ridz1208

I'm confused now whether this is intentional ? It seems like only someone with across all sites should be able to view summary of threads:
https://github.com/aces/Loris/blob/23.0-release/php/libraries/NDB_BVL_Feedback.class.inc#L460

as well as list of threads:
https://github.com/aces/Loris/blob/23.0-release/php/libraries/NDB_BVL_Feedback.class.inc#L523

[driusan]

I don't know.. I just did a git blame to try and see the history of it and that line dates from the initial commit of LORIS..

[ridz1208]

@jesscall yes, but anyone without that permission should be able to view threads created at their own site.

I don' t know much about the module but is there a way to compute what thread was created by who? or at which site ? in that case I would say current behaviour is the expected one... and we can make an improvement by addind a check for the logged in user's sites

Alternatively we can amend the testpln to specifically mention the required permission

@driusan ?

[driusan]

The thread has a UserID that created it and one of: 1. A CandID 2. a SessionID 3. A CommentID (the rest of which are null). I would say the CandID/SessionID/CommentID is more accurate for determining permissions than the user

[jesscall]

So would we proceed as follows:

If user has site affiliation to at least one cand site, they should have access?

[driusan]

I think this risks getting convoluted since it's either the CandID OR the SessionID OR the CommentID set. You're also going to lose project permissions if you just modify the query .

I think the way to do it right would be to implement the AccessibleResource interface on the NDB_BVL_Feedback class. Then it could delegate to Timepoint->isAccessibleBy (NB: depends on #7117) or Candidate->isAccessible or NDB_BVL_Instrument->_hasAccess depending on which identifier the thread has set and our access logic would be consistent with the rest of LORIS.

[jesscall]

This seems more like a new feature than a bugfix considering it's been using access_all_profiles for as long as history goes back. As well as waiting for #7117 to be merged.

If it's okay with you both, I'll remove this from 23.0-testing ?

[driusan]

Fine with me.

[ridz1208]

fine by me as well (not that you waited for my approval)

[racostas]

working on this one. the solution given seems to be working. Will do a bit more of testing.