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

Link to Billing #73

Open
wants to merge 7 commits into
base: release
Choose a base branch
from

Conversation

jan-krueger
Copy link
Contributor

@jan-krueger jan-krueger commented Feb 14, 2021

As requested in #16 I have added the first iteration of this feature. It is still fairly buggy and needs some more work. Right now it just opens the PDF of the invoice when you click the "Retrive Invoice" button.

The biggest issue is figuring out what bill belongs to which item. Right now, I parse the date of the bills and the dates of the items and when there is only one match for each date then that bill is assigned to that item. If there are more matches then I try to compare names but that doesn't always work, so this issue will work really poorly for people who have a lot of bills on a single day. Furthermore, I somehow broke some boxes and you cannot open.

So this PR is still very much a WIP but it already is in a state where it kinda works. So definitely don't merge this just yet.

Feature Preview

(When the PDF window opens it is bigger than that but I made it smaller to hide some personal details :))

  • Fix issue that some items don't collapse correctly any more
  • Improve the way it searches for the correct bill
  • Make GET requests to billing page async
  • General performance improvements

@jan-krueger jan-krueger marked this pull request as draft February 14, 2021 19:10
@jan-krueger jan-krueger marked this pull request as ready for review February 16, 2021 01:18
Comment on lines 21 to 34
} else if(bills.length > 1) {
for(let i = 0; i < bills.length; i++) {
if(bills[i]['name'].includes(name.toLowerCase())) {
return bills[i];
}
}

let fuse = new Fuse(bills, {keys: ['name']})
let results = fuse.search(name);

if(results.length > 0) {
return results[0];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How big is the hangar you're testing with?

I know some people have a few thousand items in their hangar, and grey-marketeers also have considerable invoice history - just some things to consider. I haven't had a chance to do a full review yet, just doing a quick read over the code before bed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

~50ish items. - I agree that this is not ideal but I don't think there are any other hints then the name and the date to match an item with an invoice, and considering that this gets only called once when they open a specific item this should not be too big of a problem. But it would be great if someone with a large hangar could test the performance.

Comment on lines +14 to +16
// TODO should this be place somewhere else? - it just needs to be done before we process the items
// so that we have the billing data already in order to link to it
HangarXPLOR.Billing.LoadData();
Copy link
Member

@peter-dolkens peter-dolkens Feb 20, 2021

Choose a reason for hiding this comment

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

If we're only linking to billing data - perhaps the lookup could be executed on-click rather than pre-loaded?

Might help with larger hangars/billing history

EDIT: I see in ProcessItem you may already be doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. The look up happens when you click the arrow button to open up each item. The look ups should also always be fairly fast unless the user has a lot of bills on one specific day.

 - remove invoice link for items that have no value
 - fix: always search for the name
 - find invocie for upgraded items
@jan-krueger
Copy link
Contributor Author

I have made a few changes in this commit. At one point in the code, I simply checked if only one bill exists at a given date and if so I would simply return that bill. That is not correct since CIG sometimes hands out items for special occasions without generating an invoice, so if you buy an item on the same day it would have also shown that bill for the free item. Now I always search to see if the name matches.
I also added an invoice button for upgrades. Previously, it would only show an invoice button for the base item. The issue with this right now is that it cannot "follow" upgrade chains, so it will only find the invoice for the base pledge right now.

@peter-dolkens peter-dolkens added this to In Progress in Maintenance Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Maintenance
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants