search

HabitRPG / habitica-chat-extension

A habitica.com Chat Client for Chrome
19 stars 13 forks source link

Stop query if user Id & api not set. Slowed down no chat windows notifications #39

Closed cTheDragons closed 5 years ago

cTheDragons commented 5 years ago

Fix for Issue #38

cTheDragons commented 5 years ago

Have asked @Alys in the guild on the best method of testing this.

cTheDragons commented 5 years ago

Thanks for your patience with the multiple commits. I forgot to update the firefox zip and only realised when getting ready for bed. Should be done now till the testers have completed testing.

cTheDragons commented 5 years ago

Done. Updated the mainfest, zip and Readme

cTheDragons commented 5 years ago

Just a little update, I have been running some testing on the refresh rates on chat and have found if you extend the chat greater than 60 minutes it is still querying every 5 seconds if you just leave your computer open and the chat focused. I think I will need to adjust this.

@Alys et all, could you confirm are you happy with the refresh rates for timeouts less than 60 minutes?

I am planning to change the code if the timeout is greater than 60 minutes that it will proportionally increase the timeouts. Ie if you change the timeout to 120 minutes the RefreshRateFast will be every 10 seconds.

paglias commented 5 years ago

5 seconds is too frequent unfortunately, refresh rates should be of at least 60s

Il Mar 12 Mar 2019, 00:25 cTheDragons notifications@github.com ha scritto:

Just a little update, I have been running some testing on the refresh rates on chat and have found if you extend the chat greater than 60 minutes it is still querying every 5 seconds if you just leave your computer open and the chat focused. I think I will need to adjust this.

@Alys https://github.com/Alys et all, could you confirm are you happy with the refresh rates for timeouts less than 60 minutes?

  • 5 seconds if the chat windows is focus (RefreshRateFast)
  • 45 seconds if the chat window is minmised (RefreshRateMedium) I think
  • 60 seconds if the chat window not focused (RefreshRateSlow)
  • 40 seconds if there is no chat windows.

I am planning to change the code if the timeout is greater than 60 minutes that it will proportionally increase the timeouts. Ie if you change the timeout to 120 minutes the RefreshRateFast will be every 10 seconds.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/HabitRPG/habitica-chat-extension/pull/39#issuecomment-471780942, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkAV_EHvr-Bpx8Zks-vUR71GJ7XAGe4ks5vVuYHgaJpZM4bomgx .

cTheDragons commented 5 years ago

Really?

It has been like this for a long time. I think since the beginning...

Should there be some API limiting if 5s calls are too much.

paglias commented 5 years ago

Eh, yeah there should be some rate limiting but right now we don't have anything in place.

So in the meantime 5s is way too often given the server problems we're having. Habitica chat is not meant to be realtime, I understand 60s might be a little too slow, so let's make it 30s minimum if you want

(I also noticed you poll the entire user every time, would it be possible to only fetch the notifications?)

Il Mar 12 Mar 2019, 02:03 cTheDragons notifications@github.com ha scritto:

Really?

It has been like this for a long time https://github.com/HabitRPG/habitica-chat-extension/blob/a71dddd0246dd89b0af016dca7c9216d9d12deb7/mainChat/chat_inPage.js#L386. I think since the beginning https://github.com/HabitRPG/habitica-chat-extension/blob/51b6389c1a7c08fff617f6145c449a2792834b4a/chat_inPage.js#L319 ...

Should there be some API limiting if 5s calls are too much.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/HabitRPG/habitica-chat-extension/pull/39#issuecomment-471803710, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkAV_vy4DV7IArcwPgkWeN3YYE4Yusqks5vVvz9gaJpZM4bomgx .

cTheDragons commented 5 years ago

Will do. How do you fetch notifications?

The API Doc only has posts options

paglias commented 5 years ago

Sorry, I thought there was a route to only read notifications but looks like I got confused!

Il Mar 12 Mar 2019, 09:32 cTheDragons notifications@github.com ha scritto:

Will do. How do you fetch notifications?

The API Doc only has posts options https://habitica.com/apidoc/#api-Notification-ReadNotifications

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/HabitRPG/habitica-chat-extension/pull/39#issuecomment-471903369, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkAV6yuN7WsCS-SvZfFUXDTQdoE7jLsks5vV2Y7gaJpZM4bomgx .

cTheDragons commented 5 years ago

Is there are nice route other than user that will allow me to read notifications other than user?

paglias commented 5 years ago

No but you can use the userFields parameter to only get the needed fields instead of the entire user

Il Mar 12 Mar 2019, 09:42 cTheDragons notifications@github.com ha scritto:

Is there are nice route other than user that will allow me to read notifications other than user?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/HabitRPG/habitica-chat-extension/pull/39#issuecomment-471905642, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkAVzsRdqN-yeSyVtwv41SJGf-g_YSVks5vV2hcgaJpZM4bomgx .

cTheDragons commented 5 years ago

All done. (Including the readme file)

Will submit it back to the guild for testing.

Alys commented 5 years ago

@paglias there's a group of up to 15 socialites and mods who are available to test this. Is it okay if testing happens now or do you want to set up any monitoring in advance, or wait for anything else?

paglias commented 5 years ago

Testing now is fine

Il Mar 12 Mar 2019, 12:21 Alys notifications@github.com ha scritto:

@paglias https://github.com/paglias there's a group of up to 15 socialites and mods who are available to test this. Is it okay if testing happens now or do you want to set up any monitoring in advance, or wait for anything else?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HabitRPG/habitica-chat-extension/pull/39#issuecomment-471960487, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkAV7A4kuuj5KAkpX-NeBJV5qPLoGp5ks5vV43JgaJpZM4bomgx .

cTheDragons commented 5 years ago

This can be released.

Confirmed by the following users:

Let me know if you require anything else.

Thanks

paglias commented 5 years ago

thanks @cTheDragons ! There's just one thing we'd like you to add before this can be released: when making calls to the API, could you pass another header (alongside the x-api-user and x-api-key) to make it easier to identify requests coming from the chat extension? Something like x-api-source with a value of chat-extension should work

paglias commented 5 years ago

Actually regarding the header name, let's use x-client

cTheDragons commented 5 years ago

Changes completed. Just waiting on testing to occur with Socialites.

cTheDragons commented 5 years ago

Found some issues. These have been corrected. Retesting with 2.2.1.

cTheDragons commented 5 years ago

Version 2.2.1 ready to release.

This solves issue #38.

Confirmation from

It should be noted that both with @aspiring_advocate on Firefox and @Ceran on Chrome both encountered issues

However in both these cases it is believe this was due to server issues at the time, as further tests this did not occurred. (The system no longer re-queries if it cannot load the group list which explains this behaviour).

paglias commented 5 years ago

Thanks @cTheDragons ! Will briefly test locally and release