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

Buying hat from firework shop only requires autoSatisfyWithNPCs #1528

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

Veracity0
Copy link
Contributor

A fireworks hat is an NPC-only quest item.
It should only require autoSatisfyWithNPC.
KoLmafia.makePurchases erroneously discarded the NPCPurchaseRequest since there was not an unlimited stock.

Fix: have it actually look for an NPCPurchaseRequest to decide it found one, rather than requiring an unlimited inventory to signal that.

@Veracity0 Veracity0 requested a review from a team as a code owner February 12, 2023 22:20
@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #1528 (7b27aa3) into main (3d1455f) will increase coverage by 0.02%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1528      +/-   ##
============================================
+ Coverage     34.86%   34.88%   +0.02%     
- Complexity    17524    17546      +22     
============================================
  Files          1065     1065              
  Lines        163432   163434       +2     
  Branches      34936    34936              
============================================
+ Hits          56974    57011      +37     
+ Misses        96902    96861      -41     
- Partials       9556     9562       +6     
Impacted Files Coverage Δ
src/net/sourceforge/kolmafia/KoLmafia.java 37.23% <0.00%> (+0.37%) ⬆️
.../sourceforge/kolmafia/session/ResultProcessor.java 28.30% <0.00%> (+0.13%) ⬆️
...urceforge/kolmafia/request/NPCPurchaseRequest.java 20.00% <0.00%> (+0.28%) ⬆️
...ceforge/kolmafia/persistence/NPCStoreDatabase.java 51.29% <0.00%> (+0.64%) ⬆️
...sourceforge/kolmafia/session/InventoryManager.java 36.08% <0.00%> (+1.86%) ⬆️
...ourceforge/kolmafia/request/MallSearchRequest.java 67.98% <0.00%> (+3.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d1455f...7b27aa3. Read the comment docs.

Copy link
Member

@midgleyc midgleyc left a comment

Choose a reason for hiding this comment

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

One question, but I think this is correct.

@Nested
class LimitedNPCItems {
@Test
public void willRetrieveIfCanUseMall() {
Copy link
Member

Choose a reason for hiding this comment

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

If autoSatisfyWithMall were true and autoSatisfyWithNPCs false, would purchasing work (I assume not)? Does willRetrieveIfCanUseMall use a different code path to willRetrieveIfCanUseNPCStore? Should it, given that you can't buy the fireworks hats in the mall?

Copy link
Contributor Author

@Veracity0 Veracity0 Feb 12, 2023

Choose a reason for hiding this comment

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

It does not use a different code path.

    boolean shouldUseMall = !forceNoMall && InventoryManager.canUseMall(item);
...
      boolean onlyNPC = forceNoMall || !InventoryManager.canUseMall();
      List<PurchaseRequest> results =
          onlyNPC ? MallPriceManager.searchNPCs(item) : MallPriceManager.searchMall(instance);
...
  public static boolean canUseMall(final int itemId) {
    return ItemDatabase.isTradeable(itemId) && InventoryManager.canUseMall();
  }

Since the item is non-tradable, it always goes to the NPC store and the only PurchaseRequests are from the NPC store.

@Veracity0 Veracity0 merged commit 90e4d2b into main Feb 12, 2023
@Veracity0 Veracity0 deleted the fireworks-shop-fix branch February 12, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants