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/setup #2

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Feature/setup #2

wants to merge 43 commits into from

Conversation

larasmorningtrain
Copy link
Collaborator

No description provided.

@@ -1,43 +1,34 @@
# :package_description
# PHP SDK for integrating with e-conomic for woocommerce
Copy link
Member

Choose a reason for hiding this comment

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

Det er ikke en SDK når vi har specifikke implementerede features.

"name": ":vendor_slug/:package_slug",
"description": ":package_description",
"name": "morningtrain/woocommerce-economic",
"description": "This is my package woocommerce-economic",
Copy link
Member

Choose a reason for hiding this comment

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

Lav en ordentlig beskrivelse

```

### Woocommerce
TODO: add documentation for woocommerce usage
Copy link
Member

Choose a reason for hiding this comment

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

Kan vi ikke få dokumentation allerede?

### Filters

```php
TODO: Add documentation for filters
Copy link
Member

Choose a reason for hiding this comment

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

Kan vi ikke få dokumentation allerede?

"keywords": [
":vendor_name",
":package_slug"
"Morning-Train",
Copy link
Member

Choose a reason for hiding this comment

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

morningtrain


class WoocommerceEconomic
{
public static function init(): void
Copy link
Member

Choose a reason for hiding this comment

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

Ville være rart at få delt alt dette ud, da du blander gateway, invoice creation mm. sammen, på sigt er det selvstændige features.


class OrderService
{
public static function createInvoice(\WC_Order $order, $paymentMethod): void
Copy link
Member

Choose a reason for hiding this comment

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

Generelt set i denne funktion:
Der bør være error handling undervejs, hvis noget af det har værdien null eller fejler på anden måde.


private static function getVatZone($paymentMethod): ?VatZone
{
$vatZone = apply_filters('woocommerce_economic_invoice_get_vat_zone', null);
Copy link
Member

Choose a reason for hiding this comment

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

Generelt vil gerne have vi køre filter efter formatet "NAMESPACE/CONTEXT/NAME"

F.eks. "woocommerce-economic/order/vat-zone"

Og så vil jeg have du sender flere oplysninger med f.eks. ordren, betalingsmetoden og evt. nogen af de andre informationer.

Og det er fint nok at have en shortcut filter som her, hvis det ellers er en tung proces at finde den oprindelige, men ellers vil jeg hellere have filteret ind efter den oprindelige er fundet, så man har den info med også.

Og så ser jeg ingen grund til at holde disse metoder private.

{
$this->form_fields = [
'enabled' => [
'title' => __('Aktiver/Deaktiver', 'mt-wc-economic'),
Copy link
Member

Choose a reason for hiding this comment

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

Brug "woocommerce-economic" som namespace til translations og skriv tingene på engelsk


private function getLayouts(): array
{
return Layout::all()->mapWithKeys(function ($layout) {
Copy link
Member

Choose a reason for hiding this comment

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

Burde måske ligge i en service for sig selv. og inkluderer error handling.

Gælder også de andre economic getters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants