Improve imports #15898

bryanlopezinc · 2024-11-28T10:52:41Z

PR Summary

Validate input options when importing items from the CLI.
Validate the import type to prevent errors when an invalid type is provided, applicable to both CLI and API endpoints.
Fixed an issue in the import command where a success message was returned even when the import file did not exist.
Remove the deprecated method fetchOne

Checklist:

I have read the Contributing documentation available here: https://snipe-it.readme.io/docs/contributing-overview
I have formatted this PR according to the project guidelines: https://snipe-it.readme.io/docs/contributing-overview#pull-request-guidelines
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

what-the-diff · 2024-11-28T10:53:11Z

PR Summary

Improved Import Command Functionality
- The object import command has been refined. We added a mechanism that checks the validity of an import file to make sure it exists and follows the rules we set. This helps in avoiding faulty operations.
- We've made it easier to create importers and included more detailed error messages if something goes wrong during the import.
New Factory Class
- A new class called 'Factory' has been created. This is responsible for creating appropriate importer objects based on what type of item we're importing. So it's a smart way to keep our import operations flexible and manageable.
Constant Class for Mime Types
- A new class accommodating supported types of import files has been created. It ensures we work with valid file types and helps avoid errors.
Defined Type of Importers
- We have introduced a way to define valid types of importers and a function to obtain these types, making it easier for future command-line operations.
Improved Request Validation
- The rules validating the types of items being imported have been improved. This ensures we only deal with valid items.
Upgraded Importer Class
- Changes were made to the import method making it easier to recognize the header row, improving the efficiency of the import operation.
Added Tests
- New tests were created to make sure our import command works well, which includes a variety of scenarios and checks. This will make it easier for us to catch any potential issues early on.
Code Cleanup
- Some minor changes were made to improve the style of the code and the clarity of the comments.

snipe · 2024-12-19T22:44:54Z

Hiya - looks like there's a merge conflict here - can you take a look?

uberbrady

Thank you so much for this! Not a lot of people use the command-line importer, but for those that do, it's important that we don't break it - and especially with your new tests, that's going to be a lot less likely in the future. I did have a couple of small issues, mostly about bits of verbiage and naming of things (which, as you probably know, is one of the hardest problems in computer science). But let me know what you think about that feedback and how you want to go, and then we'll start to do some testing on our end so that we can get this merged. This is really lovely code, thank you so much for your contribution!

uberbrady · 2025-02-11T13:55:42Z

app/Console/Commands/ObjectImportCommand.php

+        $importFile = new File(realpath($filepath), false);
+        $validator = Validator::make(
+            array_merge($this->options(), ['file' => $importFile]),
+            $this->rules(),


Since we only ever use the rules() function in one place, you could just embed them right here if you wanted. Not a dealbreaker though.

uberbrady · 2025-02-11T13:55:57Z

app/Console/Commands/ObjectImportCommand.php

+        $validator = Validator::make(
+            array_merge($this->options(), ['file' => $importFile]),
+            $this->rules(),
+            $this->messages()


Same here with the messages, but again, not a dealbreaker.

uberbrady · 2025-02-11T13:57:25Z

app/Console/Commands/ObjectImportCommand.php

-        ['user_id', null, InputOption::VALUE_REQUIRED, 'ID of user creating items', 1],
-        ['update', null, InputOption::VALUE_NONE, 'If a matching item is found, update item information'],
-        ['send-welcome', null, InputOption::VALUE_NONE, 'Whether to send a welcome email to any new users that are created.'],
+            ['email_format', null, InputOption::VALUE_REQUIRED, 'The format of the email addresses that should be generated. Options are <info>firstname.lastname</info>, <info>firstname</info>, <info>filastname</info>', null],


I haven't seen this <info> thing before in command-line stuff, but I'm sure it must work or you wouldn't have done it :) - but we'll be testing to see how it goes.

uberbrady · 2025-02-11T14:03:28Z

app/Http/Requests/ItemImportRequest.php

-        $class = ucfirst($import->import_type);
-        $classString = "App\\Importer\\{$class}Importer";
-        $importer = new $classString($filename);
+        $importer = Factory::make($filename, $this->input('import-type'));


I like this, but the name Factory here is confusing since that's usually used in the context of testing. Additionally, I feel like, based on how simple the Factory is, it could be just a static method on the Importer class - but we can't call it factory() since that's actually used in testing. Maybe something like Importer::makeImporter()? But that sounds janky. Importer::makeSpecifcImporter? I dunno. But something along those lines should probably work better.

uberbrady · 2025-02-11T14:04:20Z

app/Http/Requests/ItemImportRequest.php

@@ -38,9 +40,8 @@ public function import(Import $import)

        $filename = config('app.private_uploads').'/imports/'.$import->file_path;
        $import->import_type = $this->input('import-type');
-        $class = ucfirst($import->import_type);
-        $classString = "App\\Importer\\{$class}Importer";


I have always felt really uncomfortable with the way this works - I really appreciate that you took the time to fix it! Your way is so much better!

uberbrady · 2025-02-11T14:05:14Z

app/Importer/Importer.php

@@ -146,7 +146,7 @@ public function __construct($file)
     */
    public function import()
    {
-        $headerRow = $this->csv->fetchOne();
+        $headerRow = $this->csv->nth(0);


I really hate that the CSV people make us do this - but that's not your fault. I get the same deprecation warning in PHPStorm when I look at this line, so this is probably the right thing to do. Sigh.

uberbrady · 2025-02-11T14:07:37Z

app/Importer/MimeTypes.php

+
+namespace App\Importer;
+
+class MimeTypes


This is another one that is only used once - so we could inline it above if we wanted. But if you have plans or ideas on how to re-use this file elsewhere in the system in the future then I'm fine with it being a separate file. Again, not a dealbreaker for me. (Just, when we're troubleshooting, we often don't like to have to be flipping back and forth between a bunch of files - and we do a LOT of troubleshooting).

uberbrady · 2025-02-11T14:09:37Z

app/Importer/Type.php

+
+namespace App\Importer;
+
+enum Type: string


I'm fine with this being an enum, but I don't like the Type name here - it's just going to be too confusing. Can we rename it to ImportType or something like that? It can continue to live here - that's fine, since it really does only have to do with importing - but if you wanted to put it into App\Enums that would be OK too.

uberbrady · 2025-02-11T14:09:55Z

tests/Feature/Console/ImportItemsCommandTest.php

@@ -0,0 +1,167 @@
+<?php


LOVE THIS!!!!!!!!!!!!!!!!!!!!!!

uberbrady · 2025-02-11T14:10:22Z

tests/Feature/Importing/Api/GeneralImportTest.php

@@ -13,4 +14,21 @@ public function testRequiresExistingImport()
        $this->importFileResponse(['import' => 9999, 'import-type' => 'accessory'])
            ->assertStatusMessageIs('import-errors');
    }
+
+    public function testWillReturnValidationErrorWhenImportTypeIsInvalid()


Nice! Love more tests :)

bryanlopezinc added 2 commits November 28, 2024 10:33

imporove import items command

56a6c74

merge changes from develop

254f08a

bryanlopezinc requested a review from snipe as a code owner November 28, 2024 10:52

snipe changed the title ~~Impprove imports~~ Improve imports Dec 2, 2024

uberbrady requested changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve imports #15898

Improve imports #15898

bryanlopezinc commented Nov 28, 2024

what-the-diff bot commented Nov 28, 2024

snipe commented Dec 19, 2024

uberbrady left a comment

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

uberbrady Feb 11, 2025

Improve imports #15898

Are you sure you want to change the base?

Improve imports #15898

Conversation

bryanlopezinc commented Nov 28, 2024

PR Summary

Checklist:

what-the-diff bot commented Nov 28, 2024

PR Summary

snipe commented Dec 19, 2024

uberbrady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment