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

Purge inactive friends and avoids #541

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jauggy
Copy link
Member

@jauggy jauggy commented Dec 11, 2024

The purpose of this is to help fix Kobold's error https://ptb.discord.com/channels/549281623154229250/1310908634036703302
This user has 1000 avoids/blocks/ignores. It is basically difficult for him to trim that list down and it causing issues.

Screenshot 2024-12-11 at 3 11 04 pm

Screenshot 2024-12-11 at 3 12 42 pm

@jauggy
Copy link
Member Author

jauggy commented Dec 11, 2024

@L-e-x-o-n whenever you have to time can you please put this on integration server. I would like to test. Plan is to get Kobold to purge some relationships to see if it resolves his issues: https://discord.com/channels/549281623154229250/1310908634036703302

@jauggy jauggy force-pushed the jauggy/purge-old-relationships branch from 593928b to d638fc4 Compare December 11, 2024 04:23
lib/teiserver_web/live/account/relationship/index.ex Outdated Show resolved Hide resolved
number * 365.0 / 12

true ->
raise("Incorrect value assigned to :purge_cutoff")
Copy link
Contributor

Choose a reason for hiding this comment

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

Put the incorrect value in the message, when debugging it's really useful.
Also, what happen if you pass an incorrect value? Does the user see a nasty server error?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error should be received by the dev. The user can only choose options so would never get an error unless a future dev added an option that didn't work. So I probably need to make a test function to make sure a future dev doesn't input an option that would break.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is not really a dev here, because this value is coming from a user. Sure, using the web page as intended will not give you weird values but anyone could submit whatever they want.

Copy link
Member Author

@jauggy jauggy Dec 14, 2024

Choose a reason for hiding this comment

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

How could a user submit a value other than the options I have given them? All they have access to is a dropdown. The options available are hardcoded and set by the developer.

lib/teiserver_web/live/account/relationship/index.ex Outdated Show resolved Hide resolved
@jauggy
Copy link
Member Author

jauggy commented Dec 15, 2024

Updated based on @geekingfrog feedback. I didn't add any test for friend purge because I am using existing function made by Teifion (I just loop through them). Added test for relationship purge and other tests.

@jauggy jauggy force-pushed the jauggy/purge-old-relationships branch from f66e471 to 048b79a Compare December 15, 2024 21:23
@jauggy jauggy force-pushed the jauggy/purge-old-relationships branch from 048b79a to 1934cbb Compare December 15, 2024 21:30
@L-e-x-o-n
Copy link
Collaborator

L-e-x-o-n commented Dec 15, 2024

@jauggy deployed

I think the idea is nice but I would be a bit worried that I might delete some friends I care about when using this feature, especially when used with more recent timeframes... A confirmation popup would be nice, it would be even nicer if that popup could show the number and a list of users that would be purged (before clicking confirm).

@jauggy
Copy link
Member Author

jauggy commented Dec 15, 2024

I think when you deployed I will still in the middle of changes, so purging friends doesn't seem to work on integration server.

@jauggy
Copy link
Member Author

jauggy commented Dec 15, 2024

Added a confirm dialog for purging friends.

@jauggy jauggy force-pushed the jauggy/purge-old-relationships branch from 9a820cf to ba0bcbd Compare December 15, 2024 22:56
@jauggy jauggy marked this pull request as ready for review December 16, 2024 07:05
@jauggy jauggy force-pushed the jauggy/purge-old-relationships branch from ba0bcbd to 632651b Compare December 16, 2024 07:10
@jauggy
Copy link
Member Author

jauggy commented Dec 16, 2024

@L-e-x-o-n whenever you have time I'd like to request this be placed on integration server again. Last time I got kobold to test and it completely wiped his avoid list. https://discord.com/channels/549281623154229250/1310908634036703302/1318145574918291536

This occured probably because there's something wrong with last_login column. It's possibly always nil. I adjusted code to make it check last_login_timex instead. But I think last_login being always nil is problematic and can be looked at in a future ticket.

I have no idea why Teifion made both these columns

@L-e-x-o-n
Copy link
Collaborator

@L-e-x-o-n whenever you have time I'd like to request this be placed on integration server again. Last time I got kobold to test and it completely wiped his avoid list. https://discord.com/channels/549281623154229250/1310908634036703302/1318145574918291536

This occured probably because there's something wrong with last_login column. It's possibly always nil. I adjusted code to make it check last_login_timex instead. But I think last_login being always nil is problematic and can be looked at in a future ticket.

I have no idea why Teifion made both these columns

Deployed again

@jauggy
Copy link
Member Author

jauggy commented Dec 17, 2024

@L-e-x-o-n had to do a few more changes. When you have a chance please deploy again to integration. Thanks.

@L-e-x-o-n
Copy link
Collaborator

@L-e-x-o-n had to do a few more changes. When you have a chance please deploy again to integration. Thanks.

Done

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.

3 participants