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

fix: delivery options per carrier #595

Merged
merged 5 commits into from
Oct 11, 2021

Conversation

joerivanveen
Copy link
Contributor

No description provided.

@joerivanveen joerivanveen requested a review from a team September 24, 2021 14:11
Setup/UpgradeData.php Outdated Show resolved Hide resolved
Setup/UpgradeData.php Outdated Show resolved Hide resolved
@@ -571,6 +571,30 @@ public function upgrade(ModuleDataSetupInterface $setup, ModuleContextInterface
}
}

if (version_compare($context->getVersion(), '4.2.0', '<')) {
Copy link
Member

Choose a reason for hiding this comment

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

Kan dit monster ook in een losse functie?

Copy link
Contributor Author

@joerivanveen joerivanveen Oct 1, 2021

Choose a reason for hiding this comment

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

Dit upgrade bestand heeft het overal op deze manier gestructureerd, weet niet of het een goed idee is om dat op te gaan breken / te gaan wijzigen. Dan krijg je dat het upgradebestand vol specifieke functies gaat staan. En dan wil je die eigenlijk weer in een ander bestand zetten. Dan zouden we misschien het hele upgrade systeem op de schop moeten nemen, maar als ik het zo zie vind ik dat niet nodig omdat het heel transparant is (ook al wordt het bestand dus wel lang).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Misschien kunnen we specifieke upgrade classes maken die 1 taak regelen per upgrade. Maar soms denk ik ook dat we het ingewikkelder maken dan nodig?

etc/adminhtml/system.xml Outdated Show resolved Hide resolved
joerivanveen and others added 2 commits October 1, 2021 09:01
Co-authored-by: Edie Lemoine <edie@myparcel.nl>
RichardPerdaan
RichardPerdaan previously approved these changes Oct 8, 2021
* @param string $key
*
* @return string
*/
public function getTimeConfig($carrier, $key)
public function getTimeConfig(string $carrier, string $key):string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function getTimeConfig(string $carrier, string $key):string
public function getTimeConfig(string $carrier, string $key): string

$timeAsString = str_replace(',', ':', $this->getCarrierConfig($key, $carrier));
$timeComponents = explode(':', $timeAsString);
if (count($timeComponents) > 2) {
$timeAsString = $timeComponents[0] . ':' . $timeComponents[1];
Copy link
Member

Choose a reason for hiding this comment

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

Voor de leesbaarheid kan je ook dit doen als je wilt:

[$hours, $minutes] = $timeComponents;

return str_replace(',', ':', $this->getCarrierConfig($key, $carrier));
$timeAsString = str_replace(',', ':', $this->getCarrierConfig($key, $carrier));
$timeComponents = explode(':', $timeAsString);
if (count($timeComponents) > 2) {
Copy link
Member

Choose a reason for hiding this comment

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

Ik deeeenkk... dat >= 3 mentaal makkelijker te parsen is dan > 2

{
return str_replace(',', ';', $this->getCarrierConfig($key, $carrier));
return array_map(static function($val) {
return (is_numeric($val)) ? (int) $val : $val;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return (is_numeric($val)) ? (int) $val : $val;
return is_numeric($val) ? (int) $val : $val;

@joerivanveen joerivanveen merged commit 0fa7bdb into develop Oct 11, 2021
@joerivanveen joerivanveen deleted the MY-29556-delivery-options-per-carrier branch October 11, 2021 12:55
RichardPerdaan pushed a commit that referenced this pull request Jan 3, 2022
* fix: do not save address in address book (#587)

* fix: delivery options per carrier (#595)

* feat: agecheck for products (#590)

* feat: agecheck for products

* feat: pps settings and exporting pps order

* fix: correct tracktrace link in ordergrid (#607)

* fix: use BaseConsignment where you need an instance (#609)

* feat: add pickup disable option for mailbox packages (#611)

* feat: add option to disable pickup for mailbox packages

* feat: add insurance setting for shipments to BE (#617)

* fix: strip disallowed options from consignments belgium (#622)

* fix: strip disallowed options from consignments belgium

* fix: getExportMode may be null on the first install

* fix: label description maximum length

* regression: export age check

* regression: translations for dropoff delay

* regression: export correct weight for dpz
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