Maintenance refactor - 2nd pass#72
Conversation
|
Still have at least a couple bugs: Fixed that, I immediately found another: I do not even have watch items. I can't imagine why that would not have been found in testing. First, if booked cruise, should not get a notification if no longer on sale. I only notify if a watch list cruise is no longer available. For your other questions: I noticed a number of the API servers are hard-coded to use "royal" even though a cruise may be with Celebrity (for example, aws-prd.api.rccl.com/en/royal/web/commerce-api/catalog in get_new_beverage_price. Is that required, or should they go to the correct cruise line URL? I do not know. I do not have celbrite cruises, so do not know the URL. I bet they both resolve to same place, so no need to alter. Speaking of get_new_beverage_price, it seems to me that this function is used for all add-ons, not just beverages. If so, I recommend renaming the function to reflect that, as it's better for understanding and maintenance That is fine. it was only beverage when I first wrote it. it does alot more now Is it possible to have a Club Royale ID without having a C&A number? If not, I propose moving that check under the C&A check (again, for streamlining and maintainability). Similarly for Blue Chip and Captain's Club I do not think so. I think you need a C&A before you have a casino. same with celeb I want to understand your design: is it the intent that all guests would have the same stateroom subtype and category code in calculate_passenger_metrics? I think so, but wanted to confirm Finally, I noticed all the passenger discount qualifiers were hard-coded to 'n' when translating to the various r0* fields in check_if_room_is_available? Is that by design, or should those values use what's read in from the config YAML? |
|
I have a few watch items and they were coming up OK; it's likely that not having them was another execution path I hadn't taken. I'm not surprised: what you found are some camelCase to snake_case conversions that were unfortunately missed (I corrected them manually when refactoring, so it's hard to catch them all, especially sincethe server keys are in camelCase and have to be kept like that). They're being flagged even without watch items because of how the Python interpreter parses the script (which I don't fully understand myself). Thanks for answering the questions; I'll make those updates and submit an update. |
|
I'm still making changes based on your answers to my questions (thank you again!), but still had questions on the colors. As background, I was looking to be consistent in the codes:
The currently used definitions are (you see that RED follows a different standard): Here's how Google describes the differences:
Detailed Parameter Comparison
Key Practical Differences
So, which do you prefer, [9m style, [1;3;40m style, or mixed as it currently is? |
|
I am fine with mix, not so big on conforming to standards. Better yet, define a config.yaml OPTIONAL override for the colors. I run in home assistant and the terminal colors took a lot of trial and error to get them to display correctly on the default white background in home assistant logs. |
|
How do you envision a YAML override for the colors? I would recommend a one-word toggle to pick between the two, with your current definitions as default. Then again, I doubt anyone beyond you and I would care, so it may just be better from a UI perspective to just stick with your defaults. |
|
something like below. then if empty strings, I think will be default color. I made one minor change to script (I messed up the check in API call". should be easy to merge back in or fix after your PR colorOverride:
For the browse script, don't really care or a CLI argument |
|
Sounds great. If I may suggest, from a user interface point of view, I'd define a couple of keywords for people to use in the YAML (such as 'default', 'bright ', etc. and translate them in the code.Sent from my Galaxy
-------- Original message --------From: jdeath ***@***.***> Date: 6/5/26 7:51 AM (GMT-05:00) To: jdeath/CheckRoyalCaribbeanPrice ***@***.***> Cc: AESternberg ***@***.***>, Author ***@***.***> Subject: Re: [jdeath/CheckRoyalCaribbeanPrice] Maintenance refactor - 2nd pass
(PR #72) jdeath left a comment (jdeath/CheckRoyalCaribbeanPrice#72)
something like below. then if empty strings, I think will be default color. I made one minor change to script (I messed up the check in API call". should be easy to merge back in or fix after your PR
colorOverride:
red: "XXXXX"
blue: "YYYYY"
yellow: "XXXX"
—Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Maybe both. I think fine control is good, with the option of simple providing a theme. So its:
|
|
Hi, jdeath, |
|
I haven't had a chance to test yet. I will soon |
|
Understood; it's been a pretty busy time for me as well. As always, feel free to let me know if I can help. I did refactor Browse as well in the meantime, and made one more change to Check to just rearrange the functions to a more logical grouping (based on what they do). I was going to wait to submit until this one was merged, but can do so if you'd prefer. For my two cents, I think keeping them separate is better just from a "complexity of review" standpoint, but it's ultimately your call. |
|
Still errors and have not had a successful run yet.
If you can, update: that was a bug that should been fixed before. I'll look into the browse script later. |
|
I surprised there are still errors and deeply apologize; I hate wasting your time and really thought I'd caught them all (there must be some execution paths I'm unable to test). I've not yet pushed the other changes I was talking about, but will do so with that fix. |
|
Thanks for the update. That is certainly very frustrating, and I hope you're not losing patience with me (I don't have linked reservations, so it's hard to debug). Would you mind sharing the screen output you are seeing (redacting out personal information of course), so I can try to get as close to apples to apples as I can? |
above is the error. It did not print line number of anything else |
|
Thanks. Looking into it now... |
|
replace line 2096 with these two lines fixes linked reservations: Not sure why cabin number is none. |
|
Thanks! That gave me the right place to look, but opened up to potential side-effects, changing what's essentially read-only data from the server. Instead, I updated the WatchItemContext class to add the reservation ID and pass it in safely. The good news is that this fixed also looks to have corrected the Cabin None problem (which ended up being a side effect of the reservation ID bug). I've also written some linked reservation mocks in the unit tests to shake out what's hopefully the last of these nasty little bugs! |
|
Still does not work. Downloaded latest from your fork. Same issue as before Also looks like the loyalty number is not being used in the cruise cabin fare price check. |
|
Yeh. It finally handles linked reservations. Addon price checks work and pulls correct prices per passenger. A few things not quite the same: You are not pulling in the cabin paid price from the API nor applying discounts correctly. This was a real pain to get working, took a lot of debugging and a touch of AI to parse the next.js code: This I might have added after you started working on it, and sometimes the API is buggy |
|
Hopefully everything is fixed, but I can't test table size or check-in with live data (because my cruise is still too far out). I did add quite a few test cases (including for those two changes), so I hope I got it. Finally. 🤞 |
|
Closer, but still many issues. You got the table size, but still cabin pricing issues You are not applying the loyalty discount. You must apply the C&A number, not the email address. There appears to be a $100 discount, so need to ensure applying it correctly. You are also not comparing the price. Since after final payment you should not alarm if lower, but should still have the same output as me You: Me: Another reservation it does look like loyalty makes a $100 difference and you are not pulling the OBC. You are also not comparing the price You Me: Once you figure this out, be sure to double check logic for DP340 code is right. I cannot test that, but it is an important feature. It was tested extensively with others, so I want to not regress. Check in is also not quite right when not fully checked in. In my case the boarding time should be available, even though partially checked in. You: Me: |
…nts; associated testing and fixes for it
|
I made sure to add a DP340 test to make sure it's getting passed to the server properly. I can add other test cases if you think of others. |
|
Partial Check-in Fixed. Loyalty number still not applied for cabin price check I won't check again until you confirm everything is working. |
|
|
Your's looks good. Not sure why not checking cabin price or doing loyalty on my account. I'll try to poke around |
|
That's actually a relief. I was afraid I was still making a colossal blunder somewhere! While you were doing that, I did some more deep digging into the execution paths and found what I think is why it was bypassing those blocks for you.
Let me know if you want me to add that with the (hopefully) clean, unified code while I work on the new test cases! |
|
Here is a clue what is happening with loyalty number. then in your code, i go to the else statement: there it is: url_params.loyalty_number already had the loyalty number in it. Then you override it with two things that are both None. I do not think the booking.get("loyaltyNumber") ever has loyalty number. I think your account struct is only if you specify your loyalty number, which is optional Found another bug below causing OBC not to print. ignore strikethough
|
|
OBC is tricky. You should just print OBCs and not include in price. I probably do not print the current booking OBC, as sometimes it is not loaded. OBC is only ever us US dollars. So you cannot just remove it from the price before price checking. If you have the current OBC available, definitely print it . Perhaps if have old OBC, print a warning to check OBC differences before rebooking. |
|
You had a bug: you had paid_price_struct.get("paidPrice") Once I fixed that and the other loyalty overwriting, everything appears ok at first glace. including OBC Please fix those bugs and resubmit |
|
A couple other things: If the cruise price is lower, the (now 4544.66 USD) should happen after the color reset so it is in the standard color. Your code does this right for the items, but not the cruise booking |
- Removed bad loyalty_number override - Added booked vs. live OBC extraction for future use (no functional change) - Corrected paid_price key casing - Fixed final_payment_date integer casting - Corrected terminal color reset sequence
|
Thanks so much for the feedback; it helped pinpoint the problem areas. I've made those changes, enhanced the test scripts, and should be all good now (famous last words, I know...) As a note, I've left in the code that extracts booked vs. 'live' (scraped from the server for a rebooking) OBC for potential future use, but left it functionally alone (not part of any price calculation, and still printing the live one. Standby on a check: I think I see one more thing after push... 😢 |
|
Caught and corrected. Here's my output for verification:
One other question: I noted that the discount for 'fire' (r0s) is always set to 'n' despite what may be set in the config YAML; I'm curious as to why? I've put the hook in for using it dynamically in _build_checkout_url() should you want to activate it one day. |
Please note the Pending Questions section at the bottom. These were things that I came across but didn't want to change without your input or decision.
Description
This PR introduces an architectural modernization of the cruise tracker script focused on DRY (Don't Repeat Yourself) principles, rigid type-safety boundaries, clean multi-path request unification, and enhanced runtime resilience against unstable remote API responses.
Key Enhancements & Structural Themes
-- Fixed a casing typo (self.all_Included = False) inside apply_overrides that accidentally mutated an unmapped property instead of updating the actual dataclass tracker flag.
-- Cleaned up redundant buffering infrastructure (self._buffer) from the PrintRedirector log class.
-- Standardized local variables, dictionary keys, and function configurations across the entire script to meet uniform PEP 8 snake_case patterns.
Pending Questions (Technical Debt)
aws-prd.api.rccl.com/en/royal/web/commerce-api/cataloginget_new_beverage_price. Is that required, or should they go to the correct cruise line URL?get_new_beverage_price, it seems to me that this function is used for all add-ons, not just beverages. If so, I recommend renaming the function to reflect that, as it's better for understanding and maintenancecalculate_passenger_metrics? I think so, but wanted to confirmcheck_if_room_is_available? Is that by design, or should those values use what's read in from the config YAML?