Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VUFIND-1598] Allow users to view their proxy relationships #3006

Merged
merged 10 commits into from Sep 7, 2023

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented Jul 27, 2023

This PR extends the work started in #2685, adding the ability for users to view their proxy relationships. It currently includes this information at the bottom of the existing profile page for simplicity's sake, but this could fairly easily be moved to a separate page if there were a reason to do so -- we would just need to add logic to display the new menu entry at appropriate times.

I'm interested in feedback on method names, UI, labels, etc.

Here's what the proxy list currently looks like with the Demo driver:

image

TODO:

  • Implement basic UI for display
  • Add Demo driver support
  • Discuss method names and UI labels and clarify if necessary
  • Separate user interface into two sections, instead of current two-column layout
  • Add getProxyingUsers method to FOLIO driver
  • Start KohaRest implementation as a separate PR after this is merged
  • Resolve VUFIND-1598 when merging

@demiankatz demiankatz added this to the 9.1 milestone Jul 27, 2023
@demiankatz demiankatz marked this pull request as draft July 27, 2023 11:37
@EreMaijala
Copy link
Contributor

@demiankatz Great start! I'd make the methods return an array that'll easily allow at least adding of paging. Otherwise we could just include the lists in the getMyProfile response (which we could also support as an alternative so that we only need one API call if it can return all the patron information). I think there can be cases where one user manages e.g. a number of home service users and the lists could grow long.

I wonder if we could do eliminate the "proxy" term from the UI, as I'm not sure if its meaning is always understood. What do you think about e.g. "Managed Accounts", "Users Who Can Manage Your Account" and "Users Whose Accounts You Can Manage" or something like that? I'd perhaps also put the lists in a single column.

@demiankatz
Copy link
Member Author

@EreMaijala, thanks for the feedback! A few responses:

1.) getProxiedUsers was introduced in release 9.0, so if we want to change the interface now, we have to modify the spec. Since getProxiedUsers was designed to populate a drop-down list, adding pagination to it also introduces potential UI complications. I designed getProxyingUsers to match getProxiedUsers to keep things simple and intuitive. I see the validity of your points, but I think the question is whether we start with a simple implementation of both methods, and then upgrade them to more complexity in the future when a known use case arises, or if we do that work now in the interest of adding flexibility early. I suppose another option could be to add a getAllProxiedUserDetails method, and keep getProxiedUsers as it is -- then drivers could implement one method as a wrapper around the other (or vice versa) depending on implementation details. I don't have strong opinions one way or another -- just wanted to be sure the current state of work was clear.

2.) I agree that "proxy user" is a potentially confusing term -- I don't like it either. However, I'm not sure what the best alternative terminology might be. I'm not sure if "managed accounts" exactly captures it -- this sounds like I can change settings on another user's account, but it doesn't obviously say to me that I can check out books on that user's behalf. It sounds a little nicer than proxy, but I'm not sure that it's any more or less clear.

3.) You mentioned having some of this functionality in your own Koha implementation. If that's sufficiently reusable, would you be interested in trying to introduce that as part of this PR, or should we just finish up the Demo and FOLIO implementations here, and address Koha separately? (If separate is preferable, I suppose you could start with an implementation of getProxiedUsers in its current form independently of this PR, if that makes sense as an approach).

@EreMaijala
Copy link
Contributor

@demiankatz Right, maybe it's better to forget the paging stuff for now.

I think we can finish the Demo and FOLIO implementation here and add Koha separately. I don't see any issues there apart from having to make the second API call (the Koha REST plugin could return all information about the user in a single call, but maybe the driver could also cache it for a few seconds).

@demiankatz
Copy link
Member Author

Sounds good, @EreMaijala. And hopefully we can get some other opinions on the naming/layout -- I have no particularly strong opinions and am entirely open to change, but I'd like to build some consensus before taking time to make adjustments.

@maccabeelevine
Copy link
Member

Judging by the screenshot above (I'm not running it myself):

  • I don't personally like the two column layout for the two proxy relationships; even though the headings (scope=col) are clear, it may visually imply a relationship between columns i.e. User 1 and User 3 in the screenshot. However I see that the code only displays a column if it is relevant, and in the real world I think it never will be -- what is the use case for someone who both proxies and is proxied? So it's a moot point and I think the UI is fine.
  • Like @EreMaijala I don't love the word "proxy" for this, but it's what I'm familiar with from Voyager days. Maybe poll the code4lib slack on what lingo various ILS's use for this?

@demiankatz
Copy link
Member Author

Thanks, @maccabeelevine -- that's two votes for getting rid of the two-column view, and your justifications make sense. This, combined with the fact that a real-world user with entries in both columns is likely very rare, seems like a strong enough argument to make the change. I don't have time at the moment, but I have added a TODO checkbox and will take care of it when I'm able to get back to this.

Good idea to try polling Code4lib regarding language. I'll see what that turns up!

@demiankatz
Copy link
Member Author

Courtesy of Chad Nelson in Code4lib Slack:

In Outlook it uses the terminology "Delegates" and clarifies with "Delegates who can act on my behalf" which I think could be less jargon-ish than proxy user.

Looks like our research management system (Symplectic) uses "delegate" as well for someone who can deposit or claim research items on someone else's behalf.

@EreMaijala
Copy link
Contributor

@demiankatz "Delegates" sounds a lot better to me. :) And I think I can find translations for it as well.

@demiankatz
Copy link
Member Author

@EreMaijala and @maccabeelevine -- I've made some revisions to the language file and display. How do you feel about this?

image

Some notes:

  • I opted for an h3/h4/ul markup pattern here instead of the table we had previously. I imagine we could apply a class to the ul to make it look nicer, but I don't have strong opinions about how it should look.
  • I used the "Delegates" label for the section, and added more descriptive sub-labels for the "proxy of / proxy for" sub-sections. I find it very difficult to come up with good language for the "users you can proxy" list, because things get very awkward very quickly (i.e. just reversing the other language to "Users On Whose Behalf You Can Act" sounds incredibly awkward). I also thought it might be worth including the word "Proxy" somewhere in here because the existing work from FOLIO: support proxy user requests #2685 uses that word, so it seems to make sense to try to contextualize it within a broader description here.

Needless to say, I'm not overly attached to any of this, but I hope it's at least an incremental improvement over the previous revision. Please let me know if you have ideas for making it even better!

@demiankatz
Copy link
Member Author

I guess one more parallel construction might be:

Users Who Can Act on Your Behalf (Proxy You)
Users You Can Proxy (Act on Their Behalf)

That gets both the word "proxy" and clarifying text into both scenarios. But maybe that's more than we need!

@maccabeelevine
Copy link
Member

maccabeelevine commented Sep 1, 2023

I like the layout. And I like the "place requests on behalf" language. I don't love the non-parallel language. Here's another option, though I'm not sure I like it -- to fit some of the explanation below the heading:

Delegates

A delegate can place requests, checkout items, ...whatever description... on behalf of another user.
##You are a Delegate for These Users
##These Users are Delegates for You

@demiankatz
Copy link
Member Author

demiankatz commented Sep 1, 2023

Thanks, @maccabeelevine -- your approach certainly seems like a possibility, and I do see value in adding a description under the top-level heading. However, I feel like saying "you are a delegate for..." is potentially confusing, because it's easy to get mixed up about the direction of the relationship (just as when using the word "proxy"). The language does have a clear meaning, but it's easy for end users to fail to understand that meaning and get things backwards. I think it's more clear to use language that says what you can do rather than what role you hold, since that's less prone to misunderstanding. I do like the explanatory text, though. That might also offer an opportunity to drop in the word "proxy" in an instructive way.

Maybe something like this works better, though it's significantly more verbose and might also be awkward in the common situation where there is only one value (i.e. is it a problem to say "users" when there is just one? Is it worth the effort of creating singular/plural descriptions, or is that overkill? Is there a better way to make the language more quantity-neutral?):

EDIT: I originally had headings with descriptive text for each of the sub-sections, but then I realized that my headings were still confusing, so I just eliminated them in favor of the descriptive text. This is shockingly difficult -- I suspect design flaws in the English language. ;-)

Delegates

A user's delegate has permission to place requests (called "proxy requests") on their behalf.

You can place requests on behalf of these users:

  • User 1
  • User 2

These users can place requests on your behalf:

  • User 3
  • User 4

@maccabeelevine
Copy link
Member

I really like this last approach. It won't be so verbose in practice since I think each of the two tables is hidden if it doesn't apply.
I'm not sure how the 'edit' action is linked from the view page, but you probably need some "no delegate relationships setup" message if neither table applies, which will be the case for 99.99% of users :)

@demiankatz
Copy link
Member Author

@maccabeelevine, at present there is no edit option -- that's managed through the ILS and (at least in Villanova's case) is not a self-service function. There's probably some potential to add self-service, but I don't have a use case yet. So for the moment, the intent is to display nothing unless a relationship is defined. As you say, if we do add the ability to edit, we'll need to make some further revisions... but even if we do go down that road, I would envision it being a configurable option, with view-only being the default (since all of our other self-service features that actually impact data in the ILS are also opt-in, to avoid unexpected data changes).

In any case, I'll update the code to match my latest proposal in a moment...

@demiankatz
Copy link
Member Author

Okay, updated as promised. Here's a screen shot of the code in action:

image

Interested in whether @EreMaijala has any further thoughts, or if @crhallberg has aesthetic suggestions.

Copy link
Contributor

@crhallberg crhallberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The headings look in order and the markup is clear and semantic. I don't see any reason to make this fancier. Adding a class to the lists might make it easier to do this in the future, but it can be just as easily done in the future.

@EreMaijala
Copy link
Contributor

While the current proposal looks good in the context of Folio and its proxy request functionality, adding a description defining the role is problematic wrt. other systems. For instance one of the ILS systems we support allows the one acting on behalf of the other to pay their fines, but to place holds or do anything else the actual library card of the proxied user is needed. Yes, seems a bit arbitrary, but hey..

If we want to display a description, I think we need to support retrieving the capabilities or the description translation keys from the ILS. I don't have a problem with placing holds being the default as long as it can be overridden per ILS when using the MultiBackend driver.

@demiankatz
Copy link
Member Author

demiankatz commented Sep 4, 2023

@EreMaijala, I guess a related question is whether capabilities are defined at the ILS level or on the user level. Is it possible that a single user could have multiple proxies for different activities? This could get very complicated very quickly! And right now, VuFind assumes that the presence of proxied users indicates the ability to place requests. Do we need to add a mechanism to "opt out" of that functionality, or do we want to define the methods in such a way that they automatically imply a certain capability, at least by default?

I'm very short on time and want to get this done soon so that we can get translations started for release 9.1. I think this leaves us with a few possible paths forward:

1.) Add a mechanism to have the ILS driver provide the labels/translation strings, as you suggest (or provide the strings from the ILS Connection class, but check the ILS driver for overrides). This seems like it might work as a short-term solution, but I'm a little concerned that this will end up being somewhat redundant if in future we add a more semantic proxy capability check to the driver to control conditional proxy-related functionality. Of course, turning semantic capabilities into a translation string is also potentially complex and problematic too.

2.) Make the language even more generic so that we don't have to build more code. Something like:

Delegates

A user's delegates have permission to perform some actions on their behalf.

You are a delegate for these users:

  • User 1
  • User 2

These users are your delegates:

  • User 3
  • User 4

This gets us back into the slightly more confusing territory I had tried to avoid, but maybe it's tolerable. And of course it's easy enough to override the description locally with something more specific if necessary, and in future we could add greater smarts to it, like a list of allowed actions (which could be at the description level, or at the user level, depending on the necessary granularity).

3.) Punt the whole thing to 10.0 and completely redesign everything, because the current implementation of proxy requests added in 9.0 already seems to have shortcomings.

If we can agree on language, I lean towards option 2, since it seems like a workable compromise that doesn't require a lot of additional development time. In the long term, I'd of course like to do something more sophisticated, but given the incredibly high volume of reviews right now, I don't realistically see myself having much time to write code between now and the 9.1 release.

@EreMaijala
Copy link
Contributor

@demiankatz I'm ok with option 2. Anything else seems pretty complicated, and trying to be prepared for everything seems difficult.

@demiankatz demiankatz marked this pull request as ready for review September 6, 2023 19:22
@demiankatz
Copy link
Member Author

Thanks, @EreMaijala! I've adjusted the language to match option 2 as discussed, and I have also added the missing FOLIO implementation. I think this is ready to merge if you are happy with the current language and @maccabeelevine agrees and thinks the FOLIO implementation looks reasonable.

Copy link
Member

@maccabeelevine maccabeelevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm fine with that generic language.

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@demiankatz
Copy link
Member Author

Thanks, everyone! Merging now -- @EreMaijala, please open a KohaRest PR to add proxy functionality if you think it's worth it and if time permits... but no pressure, of course!

@demiankatz demiankatz merged commit 72f3404 into vufind-org:dev Sep 7, 2023
7 checks passed
@demiankatz demiankatz deleted the vufind-1598 branch September 7, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants