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

[Feature] Reserve multiple bags at once #567

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

ihor-chaban
Copy link

@ihor-chaban ihor-chaban commented Dec 2, 2024

Updated reservations logic to allow reserving multiple bags at once, referring #510
Please note that this PR includes #564 as well because reservations are currently broken.
If you don't want to split reservations fix and reservation of multiple bags into separate PRs, you can merge only this PR and drop #564.
Docker images I used for testing are available at gorus5/tgtg-scanner:latest-reserve-multiple-bags and gorus5/tgtg-scanner:latest-alpine-reserve-multiple-bags (multi-arch).
I tried reserving multiple bags, creating and canceling orders with multiple bags in different real-life scenarios; everything worked fine.

Explanation of the feature:
Currently, each reservation consists of 1 bag only. So, if you want to reserve multiple bags from the same store it will create many separate reservations with 1 bag each. This means that it will reserve only 1 bag per scan and you'll have to wait until the next scan to reserve the next bag (they might be sold out until the next scan and you'll also need to cancel and buy each bag separately).

My change allows creating reservations with multiple bags at once:
To reserve several bags from the same store click several times on this item as before.
If a reservation/order consists of more than one bag it will be shown respectively e.g. Starbucks (2 bags)
When bags become available, it will reserve as many bags as you have in your queue in one order.
If there are fewer available bags than you have in your queue it will reserve all available bags and the reservation will remain active with the remaining bags that haven't been reserved yet.
If the order fails (e.g. sold out), the reservation amount will remain the same.
Cancelling a reservation/order will delete the reservation/order with all the bags in it.

Examples:

  1. You want to reserve 2 bags, 4 bags become available -> it will reserve 2 bags and delete the reservation.
  2. You want to reserve 3 bags, 1 bag becomes available -> it will reserve 1 bag and the reservation remains active with 2 bags.
  3. You want to reserve 3 bags, 1 bag becomes available but the order fails -> the reservation remains active with 3 bags.
  4. You want to reserve 1 bag, 1 bag becomes available -> it will reserve 1 bag and delete the reservation.

Important!
Based on my testing, certain stores may have a maximum limit on bags per customer.
Please ensure that the desired quantity for reservation from this store is allowed first.
Otherwise, your reservation will keep failing with this log message (until canceled or reserved in smaller chunks):

WARNING  Order failed: (200, b'{"state":"OVER_USER_WINDOW_LIMIT"}')

Pull Request Checklist

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Did you make your Pull Request on the dev branch?
  • Does your submission pass tests? make test
  • Have you lint your code locally prior to submission? make lint
  • Could you build and run the docker images successfully? make images
  • Could you create a running executable? make executable
  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran manual tests with your changes locally?
  • Have you updated the documentation for your changes, as applicable?

@ihor-chaban ihor-chaban force-pushed the reserve-multiple-bags branch from d12be9f to a589c92 Compare December 6, 2024 17:08
@ihor-chaban ihor-chaban changed the title Reserve multiple bags at once [Feature] Reserve multiple bags at once Dec 9, 2024
@danwie
Copy link

danwie commented Dec 11, 2024

Would it be possible to put the number before the bag name instead of after? When on my phone the number is truncated in Telegram. Also, I think it would be useful to put "1" if only one bag is reserved.

@ihor-chaban
Copy link
Author

ihor-chaban commented Dec 11, 2024

@danwie thanks for your feedback. I've noticed this issue as well even before my changes.

I think that the majority of reservations will still consist of 1 bag only, so I made no changes in this case not to make already long names even longer. I'm not sure if adding 1 would be useful here.

Also, I don't like putting the amount of bags in front of the store name.
There is a format similar to my change implemented in /reserve menu where the currently available amount is displayed after the colon in the end. Compared to this, the amount of bags in a reservation/order in my feature stays at the same relative position e.g. Starbucks: 2 vs Starbucks (2 bags).

Unfortunately, Telegram view is not consistent across different devices for this matter. For example, on desktop it displays long button names as Starbucks blah ... (2 bags) but on mobile it's shown as Starbucks blah blah ... (not actually truncated but goes beyond the button borders).
I hope this will be fixed in later releases.

In the meantime, I think the best solution would be to cut the middle part from long button names as it's done in Telegram desktop. But then I'll have to make it consistent everywhere and update /reserve menu to use this new format as well.

Another option might be to use ReplyKeyboardMarkup instead of InlineKeyboardButton as it allows longer and even multiline text for buttons, but it will require more changes than just updating text format because it uses a different type of buttons. See https://stackoverflow.com/questions/46917909/newline-in-telegram-inline-keyboard-for-python

I would like to get a second opinion on this change before actually implementing it.
@Der-Henning what do you think?

@ihor-chaban
Copy link
Author

ihor-chaban commented Dec 12, 2024

@danwie I added the shortening of long text on buttons with an ellipsis in the middle to a maximum of 50 characters.
Per my testing, this should be a reasonable limit to fit everything into a visible part of a button.
But, of course, it depends on your font, resolution, etc. Even the characters themselves are not monospaced, so the WWW string would naturally take more space than III. But overall it looks good for me.
Please try it also and let me know your results.

@Der-Henning I'm not sure if @staticmethod was the right choice for the _shorten_with_ellipsis method.
But it seemed reasonable to me and it works as expected. If you know a better way to refactor it, please suggest.

@ihor-chaban ihor-chaban changed the base branch from main to dev December 23, 2024 18:20
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.

2 participants