feat: Fetch and show total drops per account from riot

[rektile]

Fixes #149

Summary of Changes

The program has been updated to store and display the total number of drops accumulated by each account. This information is saved in a JSON file located in the "src/saves" folder. The format of the JSON data is as follows:

Additional context

Rektile#7582

Testing instructions

Verify that the total amount of drops gets saved and updated when the account gets a new drop.

How to download the PR for testing

Using GitHub CLI

1. Clone this PR
2. Run gh pr checkout 158 (Requires GitHub CLI)
3. Follow the Advanced Installation Guides from the Wiki

Using regular GIT

1. Fetch the PR git fetch origin pull/<PR_NUMBER>/head:<LOCAL_BRANCH_NAME> (e.g. git fetch origin pull/110/head:notif)
2. Checkout the branch git checkout <LOCAL_BRANCH_NAME> (e.g. git checkout notif)
3. Follow the Advanced Installation Guides from the Wiki

[Skribb11es]

Ideally would be an optional feature, and needs some cleaning up in places, but otherwise well done.

[alepouna]

Ideally would be an optional feature, and needs some cleaning up in places, but otherwise well done.

Why would this be optional? It's a handy feature that requires no configuration or setup from the user. Can't see why it should require a user to mess with config file, which at the end is adding more points of failure.

[Skribb11es]

Ideally would be an optional feature, and needs some cleaning up in places, but otherwise well done.

Why would this be optional? It's a handy feature that requires no configuration or setup from the user. Can't see why it should require a user to mess with config file, which at the end is adding more points of failure.

Handy for what purpose? A large majority of the users "set it and forget it", if anything it just means more code running for no real reason. Makes sense as an optional feature, because it's not important information, the farmer should ideally be lightweight as possible on its default config, allowing more advanced users to add onto it if they need/want to. The same went for the webhooks, they are also "handy" but aren't forced, yes they have the added need of providing a webhook, but they follow the same line of reasoning. If someone wants it, they should be able to add it, but if it's not necessary why worry about it?

[alepouna]

A large majority of the users "set it and forget it",

You are part of the Discord server, so you have seen the following your self, I assume. A large majority of the users "set it and forget it" a large majority of users has troubles getting the basic config file to work. Points of failure. This is a simple enhancement that adds no additional load to the core application, the performance impact is irrelevant.

The same went for the webhooks, they are also "handy" but aren't forced

The webhook configuration REQUIRES a URL to be set by the user as each webhook is unique. Comparing them with features like this PR, is wrong.

---

No further comments for the PR. LGTM.

[rektile]

So what will happen now?

[LeagueOfPoro]

I think this feature is very unnecessary. We already retrieve the full list from riot anyway. At most, we can create a switch in config that would switch between current version and total number of drops of the account.

[rektile]

I think this feature is very unnecessary. We already retrieve the full list from riot anyway. At most, we can create a switch in config that would switch between current version and total number of drops of the account.

Do you want me to modify it so that it displays the total number of drops from the Riot server? Also, should this feature be configurable, as suggested by Skribb11es?

[LeagueOfPoro]

I think this feature is very unnecessary. We already retrieve the full list from riot anyway. At most, we can create a switch in config that would switch between current version and total number of drops of the account.

Do you want me to modify it so that it displays the total number of drops from the Riot server? Also, should this feature be configurable, as suggested by Skribb11es?

Yes, optional. The current state will be the default. Then if config has something like showHistoricalDrops: True then it would show all drops.

[rektile]

I think this feature is very unnecessary. We already retrieve the full list from riot anyway. At most, we can create a switch in config that would switch between current version and total number of drops of the account.

Do you want me to modify it so that it displays the total number of drops from the Riot server? Also, should this feature be configurable, as suggested by Skribb11es?

Yes, optional. The current state will be the default. Then if config has something like showHistoricalDrops: True then it would show all drops.

Now that it is configurable, the total drops are still saved locally, which means that the total reflects the amount collected since you started using the program with that account. Should this stay the same or does this need to be pulled from the riot server?

[alepouna]

Should this stay the same or does this need to be pulled from the riot server?

If I understand correctly from what Poro said (We already retrieve the full list from riot anyway) they are already fetched, dare to say cached, somewhere in the app already. Perhaps you can just use that instead.

[rektile]

Should this stay the same or does this need to be pulled from the riot server?

If I understand correctly from what Poro said (We already retrieve the full list from riot anyway) they are already fetched, dare to say cached, somewhere in the app already. Perhaps you can just use that instead.

The browser class has a method named 'checkNewDrops' that fetches drops and returns a list of all the drops after a certain time period. I can implement it so that inside the 'checkNewDrops' method, it caches the total drops inside a field. Alternatively, I can create a new method that makes its own request.

[rektile]

If this gets merged, there needs to be an option added to the configuration wiki. I don't think i can change that in a PR

[Skribb11es]

You can clone the wiki and make the changes locally, then upload the new wiki page in a comment here, as I did for PR #104

[rektile]

Here is the updated configuration wiki. Configuration.md

[rektile]

This should fix the merge conflict.

[LeagueOfPoro]

Let's make it default. Looks pretty good. (i will do it)