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

Add enum / constants classes for Accounting and PayrollAU and a fluent whereDate to query builder #885

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Healyhatman
Copy link
Contributor

Add constant/enum-like classes for all the Types for Accounting and PayrollAU

I've added them under Enums because one day we can make them actual enums and you call them PROPERTY_TYPE_ENUM anyway. Additionally, they don't belong in Models because they're not models

All the const names are copied directly from Accounting => Types and PayrollAU => Types and Codes

I copy+pasted and did it all manually because I don't have your fancy auto-scraping powers

Mark (some?) old constant classes as deprecated and point to the new classes
Add some "@see" docblock comments pointing to the new classes

Fix return types

  • PayrollAU
    • Setting/TrackingCategory
    • Setting

Add more fluent ->whereDate function to query builder.

Usage examples

// Invoices on a particular date
$application->load(Invoice::class)->whereDate(new DateTime('2023-01-01'));

// Invoices between a particular date range
$application->load(Invoice::class)
   ->whereDate(new DateTime('2021-12-01'), '>')
   ->whereDate(new DateTime('2021-12-30'), '<');

Mark old constant classes as deprecated and point to the new classes
Fix return types
   * PayrollAU
     + Setting/TrackingCategory
     + Setting

Add more fluent ->whereDate function to query builder
@calcinai
Copy link
Owner

calcinai commented Feb 6, 2023

Did you have a suggested migration path for this? There are obviously a couple of braking changes in there, so this will need to be in a major release.

Related: there should be readme updates to use the enums instead.

A lot of work in there though, looks good.

@Healyhatman
Copy link
Contributor Author

Healyhatman commented Feb 7, 2023 via email

@calcinai
Copy link
Owner

calcinai commented Feb 7, 2023

Yeah although strictly speaking, we shouldn't be (potentially) causing fatals without a major version bump. Even for deprecations..

I see some ORGANISATION_* constants removed and some functions, too.

@Healyhatman
Copy link
Contributor Author

Sorry about that, right you are.

The functions were removed because that endpoint is GET only so those functions shouldn't be there.

I'd rather save a version bump for something cool like moving to PHP 7.4. If you prefer I can simply discard the breaking changes?

@calcinai
Copy link
Owner

calcinai commented Feb 8, 2023

Yeah probably the best, if that's ok.

I think PHP7.4 might be a waste, given it's EOL...

@Healyhatman
Copy link
Contributor Author

I'm happy to go straight to 8.1, love that

@Healyhatman
Copy link
Contributor Author

OK there you go breaking changes removed. Let me know if I missed any.

Could I leave the readme.md bit to you? Or where in it would you want me to make changes and what do you need it to say?

@calcinai
Copy link
Owner

@Healyhatman just wondering if we're deprecating the constants on the classes themselves, or what the actual forward path is with the enums–and making the sure the Readme.md reflects the usage correctly.

@Healyhatman
Copy link
Contributor Author

Healyhatman commented Feb 14, 2023 via email

@Healyhatman
Copy link
Contributor Author

Because I can if you need me to

@calcinai
Copy link
Owner

calcinai commented Feb 26, 2023

Sorry the last couple of weeks have been pretty hectic over here.

Do you need me to add other enums from the other things?

Which other things do you mean? All I was suggesting is consistency across all models etc

@Healyhatman
Copy link
Contributor Author

I would like everything everywhere to use the constants I've made, but I didn't want to remove any from the models because people might be relying on them and it'll break their code.

@Healyhatman
Copy link
Contributor Author

So whatcha reckon, what do you need I'll get it done

@calcinai
Copy link
Owner

calcinai commented Mar 8, 2023

Ok cool, well I think a few things should happen

  • Move the whereDate to a seperate PR, and that can go in right away.
  • Update the Readme.md to reflect the usage of the enum classes, rather than the model constants
  • Add deprecated tags to the class constants

One last thought–why don't we go straight to actual enums, then force PHP8.1 from hereon out?

@Healyhatman
Copy link
Contributor Author

I'd love that, then we can also enforce types on the functions that warrant it.

@calcinai
Copy link
Owner

calcinai commented Mar 8, 2023

The current version is stable, so we can just put it on a legacy branch and make master PHP8. If you're happy to test that out, just break it up into a few PRs

Copy link
Contributor

@bretto36 bretto36 left a comment

Choose a reason for hiding this comment

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

This is great. Just noticed a couple that didn't match the docs


namespace XeroPHP\Enums\Accounting\Organisation;

class OrganisationType
Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.xero.com/documentation/api/accounting/types/#organisation

In the docs it has

ACCOUNTING_PRACTICE
CHARITY
CLUB_OR_SOCIETY
COMPANY
INDIVIDUAL
LOOK_THROUGH_COMPANY
NOT_FOR_PROFIT
PARTNERSHIP
S_CORPORATION
SELF_MANAGED_SUPERANNUATION_FUND
SOLE_TRADER
SUPERANNUATION_FUND
TRUST

@@ -0,0 +1,47 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

You think it's worth putting the rates here as a secondary const BLINPUT_RATE = 0


class LeavePeriodStatusCode
{
const SCHEDULED = 'Scheduled'; // The default status
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

3 participants