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

Staff Assistance Requests #222

Open
wants to merge 22 commits into
base: 6.1.0.dev
Choose a base branch
from

Conversation

abuckles-uci
Copy link
Contributor

Hello @rptmat57! This is the initial code for "staff assistance requests". It's essentially a duplicate of the buddy request system, but without needing to specify a start, end, or area. It is only a description of the issue the user wants assistance with. There's some more that I would like to add, however I would like to get your opinion if this is a good fit (and general code review).

  • Allow staff to mark a request as "resolved", which essentially just hides the request and all its replies
  • Restrict users to only see their own requests (only staff can see all the requests)

Thanks!
Aaron

Closes #186

@abuckles-uci
Copy link
Contributor Author

image

@rptmat57
Copy link
Contributor

rptmat57 commented May 8, 2024

Hi and thank you for this PR draft.
I like it and I think it would be a great addition to NEMO.

A few things:

  1. Please follow the instructions for installing dev-tools and pre commit hooks: https://github.com/usnistgov/NEMO/wiki/Development-Setup#set-up-pre-commit-hooks and then run it on all the files so your changed files are formatted correctly
  2. The staff assistance feature would need to be enabled explicitly, following the same idea as the customization "adjustment_requests_enabled"
  3. There are a few copy-paste errors from the buddy view, so make sure to double check and also add new css classes for staff assistance.
  4. You can remove the preference "email_new_staff_assistance_request_reply" since in this case the creator is the only user and he will always need to be notified

@abuckles-uci
Copy link
Contributor Author

Thank you for reviewing! I'll implement these changes

@abuckles-uci
Copy link
Contributor Author

Hi @rptmat57,

I've implemented the requested changes and finished up on adding a button to resolve requests. I made it so requests can only be resolved by staff and only if there is at least one reply. In addition, users can now only see their own requests, unless they are staff in which they see all.

The UI is pretty rough still, so I was hoping I could get some suggestions:

  1. In the image below you can see that the top request scrolls to the right instead of wrapping. This is the same as the buddy systems. Should this be fixed?
  2. I think it would be useful to filter out resolved requests. Currently I'm showing them with a [Resolved] badge below all unresolved requests.
  3. I'm not totally sure how the notification system works, I haven't looked into it enough. I made the "expiration" 30 days like other requests, but I'm not sure if there's a better number for this.
  4. On the same note as notifications, how does the email situation look? In my opinion it would be the requestor and all staff. In addition, how can I test emails locally? I was hoping I could get some assistance here.

Thanks!
Aaron

image

@abuckles-uci abuckles-uci marked this pull request as ready for review May 9, 2024 00:09
@rptmat57
Copy link
Contributor

Hi Aaron,

  1. Yes, it should probably be fixed
  2. I would have a separate section with resolved requests, a bit like we have for approved adjustment/area access requests
  3. 30 days is good
  4. Requestor and staff sounds good, you can test emails by going to Detailed administration -> Email logs

@uci-oit-or
Copy link

Hi @rptmat57,

I've implemented the collapsible section for resolved requests. I've also added the ability for requests to be reopened by staff (in case of mistake). I fixed the word-wrap for both staff assistance requests and buddy requests. I still haven't tested the emails yet, but will do so when I have some more time.

Something I've noticed is that regular, non-staff users will see a badge if another user creates a request, even though they can't see the request themselves (as intended). Is this something that should be fixed? Would it require modifying how the notification system works?

image

I still need to do more testing, but please let me know if you catch anything in the code. Thanks!

-Aaron

Screenshot of UI:
image

@abuckles-uci
Copy link
Contributor Author

Hi @rptmat57,

I committed your suggested change, so the notifications appear to be working as intended now.

For the emails, I added a new email that sends to all staff when a staff assistance request is created. The replies still work the same (send to the request creator and other repliers). I have a couple of questions:

  1. The replies use the email_send_staff_assistance_request_replies user preference. Do we want new staff assistance requests to use this same preference or always send to all staff?
  2. Do we want an email to the request creator when their request is resolved/reopened?
  3. I also hid the "New Request" button for staff, since this feature is meant for non-staff. Is this how you want it?

Thanks,
Aaron Buckles

@abuckles-uci
Copy link
Contributor Author

Hi @rptmat57,

Have you had a chance to review this PR yet?

Thanks,
Aaron Buckles

Copy link
Contributor

@rptmat57 rptmat57 left a comment

Choose a reason for hiding this comment

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

Thanks, it looks great! Outside of my inline comments, there are a few general changes I would like to request:

  1. Please merge with 6.0.0.dev (there are new recent changes)
  2. Also, I would prefer the notification (badge) for staff to only go away when the request is resolved
  3. Lastly, when requests are resolved, let display them as a table (similar to adjustment requests, with replies smaller etc.)

@abuckles-uci
Copy link
Contributor Author

Thanks, it looks great! Outside of my inline comments, there are a few general changes I would like to request:

  1. Please merge with 6.0.0.dev (there are new recent changes)
  2. Also, I would prefer the notification (badge) for staff to only go away when the request is resolved
  3. Lastly, when requests are resolved, let display them as a table (similar to adjustment requests, with replies smaller etc.)

Sounds good, thank you for the feedback. I'll work on implementing these sometime next week hopefully.

@abuckles-uci
Copy link
Contributor Author

Thanks, it looks great! Outside of my inline comments, there are a few general changes I would like to request:

  1. Please merge with 6.0.0.dev (there are new recent changes)
  2. Also, I would prefer the notification (badge) for staff to only go away when the request is resolved
  3. Lastly, when requests are resolved, let display them as a table (similar to adjustment requests, with replies smaller etc.)

Hi @rptmat57, how does this look?

image

@abuckles-uci abuckles-uci changed the base branch from 6.0.0.dev to 6.0.2.dev June 17, 2024 16:51
@abuckles-uci
Copy link
Contributor Author

Hi @rptmat57,

I made the requested changes, so this should be ready to be reviewed again.

Thank,
Aaron

@rptmat57
Copy link
Contributor

thanks, I'll try to take a look this week

@rptmat57 rptmat57 changed the base branch from 6.0.2.dev to 6.1.0.dev July 1, 2024 14:38
@abuck351
Copy link

Hi @rptmat57, have you had a chance to review this PR yet? Thanks!

@abuck351
Copy link

Hi @rptmat57

Sorry to keep bumping this PR. Do you have any feedback? Thanks!

  • Aaron

@abuckles-uci
Copy link
Contributor Author

Hi @rptmat57,

Are there any updates for getting this pull request merged? Does anything need to be changed?

Thank you,
Aaron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants