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

Location importer via Livewire Importer #12846

Merged
merged 12 commits into from
May 3, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Apr 16, 2023

Okay, I think I've hammered out these details, but it will for sure need a lot of testing, not just from the Location Import side, but also to make sure I didn't break any of the other existing imports. I've tested a LOT, but there will always be funny use cases, of course.

Fixes #8401

Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
@what-the-diff
Copy link

what-the-diff bot commented Apr 16, 2023

PR Summary

  • Added support for importing location data
    Updated the ImportController and Importer classes to handle location imports
  • Implemented LocationImporter class
    Created a new class for managing location-specific import operations
  • Included sample location data
    Added a MOCK_LOCATIONS.csv file with 100 sample locations in Alberta, Canada

@snipe snipe added the livewire label Apr 16, 2023
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

I tried this with the included csv, a slightly modified version, and sample csvs for the other import types. Everything worked as expected besides one comment on updating I highlighted below 👍🏾

$this->importTypes = [
'asset' => trans('general.assets'),
'accessory' => trans('general.accessories'),
'consumable' => trans('general.consumables'),
'component' => trans('general.components'),
'license' => trans('general.licenses'),
'user' => trans('general.users'),
'location' => trans('general.locations'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: can we move this up a line so that the options are in alphabetical order?
select options

Copy link
Owner Author

Choose a reason for hiding this comment

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

I added it to the bottom of the list because it's less common than the others. It's far more common for users to import all of those other first-class objects versus locations, so I was trying to list it by priority, not alphabetical.

Lemme chew on that though.

app/Importer/LocationImporter.php Outdated Show resolved Hide resolved
app/Importer/LocationImporter.php Outdated Show resolved Hide resolved
snipe and others added 4 commits April 18, 2023 13:26
Co-authored-by: Marcus Moore <contact@marcusmoore.io>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
@snipe
Copy link
Owner Author

snipe commented Apr 18, 2023

Okay, can we give this another spin please?

@snipe snipe requested a review from marcusmoore April 18, 2023 23:35
@marcusmoore
Copy link
Collaborator

I'll take another look tomorrow morning 👍🏾

@snipe
Copy link
Owner Author

snipe commented Apr 19, 2023

Blargh - I think I broke something - not seeing users being created when I test with the assets sample CSV

@snipe
Copy link
Owner Author

snipe commented Apr 19, 2023

Actually, no - the sample CSV seems to not have "Checkout Type" in there. I might not have broken it after all. Will explore more tomorrow.

Copy link
Collaborator

@spencerrlongg spencerrlongg left a comment

Choose a reason for hiding this comment

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

Haven't been able to break it, all looks good to me.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

I eyed the commits added after my review and it all looks good. I think we can trust my eyes and @spencerrlongg's actual testing right?? 😅

@snipe snipe mentioned this pull request Apr 25, 2023
@snipe snipe merged commit ea17fde into develop May 3, 2023
3 checks passed
@snipe snipe deleted the features/livewire_location_import branch May 3, 2023 18:02
@FillmoreB0
Copy link

Shouldn't the .csv file include a "Parent" field to match the Locations export?

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

Successfully merging this pull request may close these issues.

Import/Sync Locations
4 participants