DeskDirector / Issues

Issue Tracker for DeskDirector
2 stars 0 forks source link

[20887] Chat sessions are appearing in the chat session manager when they should not be #148

Closed Andrew-Lahikainen closed 4 years ago

Andrew-Lahikainen commented 4 years ago

This happens on the find api:

POST /api/v2/tech/chat/sessions/find

Nness commented 4 years ago

@Andrew-Lahikainen The intention was let TECH to decide what to do with it. So every find result need to be combine with bulk get tickets results. Either display it and make it gray out, or not display it.

If server do that, the consequence have two fold, depends on implementation. Let's use skip 0 and take 20 as example.

  1. Member cannot access all sessions, server return empty list. TECH sees empty list auto goes to next page. Problem with this approach is if TECH does not do that, user will think there is no more page there.

  2. Member cannot access all sessions, server goes to next page until find a list of sections user can access. The response contain request as skip 100 take 20. That means we could already skipped 5 pages. Problem with this approach is if TECH does not recognize the instruction from the response, next page is skip 20 take 20, then TECH will display exactly same result.


Now if we account for breaking change or move API to next version we could use infinite scroll kind of design. Or delta query. That means it will give a next page query token.


Let's face it, chat data have permission itself is problematic. Especially it stored in separate data store. Also even when stored within same store, there are still requirement on joined index.

Another solution is give global chat session list only to admin. Where they are supposed to see all tickets. Then for normal engineer they can see chat session in a specific ticket.

Andrew-Lahikainen commented 4 years ago

That makes sense, I'll probably hide the chat history page to non admin (the chat histories are already on tickets anyway) and then filter out results for use in the chat manager.

On Mon, 18 Nov 2019, 7:44 AM Silverlancer, notifications@github.com wrote:

@Andrew-Lahikainen https://github.com/Andrew-Lahikainen The intention was let TECH to decide what to do with it. So every find result need to be combine with bulk get tickets results. Either display it and make it gray out, or not display it.

If server do that, the consequence have two fold, depends on implementation. Let's use skip 0 and take 20 as example.

1.

Member cannot access all sessions, server return empty list. TECH sees empty list auto goes to next page. Problem with this approach is if TECH does not do that, user will think there is no more page there. 2.

Member cannot access all sessions, server goes to next page until find a list of sections user can access. The response contain request as skip 100 take 20. That means we could already skipped 5 pages. Problem with this approach is if TECH does not recognize the instruction from the response, next page is skip 20 take 20, then TECH will display exactly same result.


Now if we account for breaking change or move API to next version we could use infinite scroll kind of design. Or delta query. That means it will give a next page query token.

Let's face it, chat data have permission itself is problematic. Especially it stored in separate data store. Also even when stored within same store, there are still requirement on joined index.

Another solution is give global chat session list only to admin. Where they are supposed to see all tickets. Then for normal engineer they can see chat session in a specific ticket.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DeskDirector/Issues/issues/148?email_source=notifications&email_token=ACQU4MO4CFEROJZXXKDZ7Y3QUGGIJA5CNFSM4JNVZXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEIS4BA#issuecomment-554774020, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQU4MN2G3O34JQ4U6IVCE3QUGGIJANCNFSM4JNVZXWQ .

Nness commented 4 years ago

Sorry wake you up on your holiday. :(

For detail we can discuss tomorrow.

Nness commented 4 years ago

Session should now have meta and with property to indicate whether given technician can access to this session or not.

Nness commented 4 years ago

This is now included in server 18.101.1

meta?: {
    canAccess: boolean
}
Andrew-Lahikainen commented 4 years ago

Reopening since front end has not been completed

Nness commented 4 years ago

This has been fixed in frontend.