Feature request: bulk import

tomvanenckevort-hp commented 6 years ago

We like using your redirects package and it has pretty much become the standard with all our Umbraco projects. But one feature we would like to see is a bulk import to be able to import a large list of redirects in one go. Often the redirects 'required' for a site are determined by the SEO team, who then provide us with a spreadsheet of old and new URLs. If there was a way to import those straight into the dashboard, it would be a real time saver.

So we're thinking the functionality should allow you to upload a CSV or XLSX file which contains the following columns:

Old URL (relative)
New URL (relative or absolute if it's an external URL)
Type (which would be Content, Media or URL)

Apart from selecting the file, the user would also select for which site (or All Sites) they want to upload the redirects, and whether they want to override existing redirects or skip if a redirect already exists.

When clicking Upload it would then parse the CSV/XLSX and then loop through each entry and add or update (if set to update) the RedirectItemRow to the database. This would also involve looking up the Umbraco node IDs of each new URL (if it's Content or Media) since that wouldn't be something the SEO team would look up.

As I said, it's something we would be very keen to see, and we're thinking of adding this ourselves through a PR. But before starting the work we wanted to check first if this functionality would be useful to other people as well, and if we need to think of any other functionality as part of this.

abjerner commented 6 years ago

That sounds awesome, and I do think it would be useful to others as well. But given the amount of active pull requests for this package (and our other packages), it's not something I have the time to add myself - so a pull request would be more than welcome.

There are however a few things we should probably think of:

It might be specific to each individual Umbraco installation whether editors should have an import option - and it might be specific for each user or user type. So we would need a way to configure that
Parsing CSV and XLSX files would most likely require one or more dependencies/assemblies. We have our own Skybrud.Csv for parsing CSV files. And we're normally using various versions of a paid third-party library for parsing Excel files, but I don't know whether the license allows us to release the DLL as part of a package. Or whether there are any free alternatives. But alone due to the dependencies, I think bulk import should be released as a separate package - but of course integrated as best as we can with the main package.

I'll try giving it some more thought, and then get back to you ;)

abjerner commented 6 years ago

@tomvanenckevort-ez Do you have some thoughts on how an import feature should work?

I have some thoughts of my own that I have tried to summarize below (right now they're just somewhat loose ideas).

The way I see it, it can either be done super simple or slightly more complex (here with CSV as an example):

For a simple approach, the redirects dashboard could have an "Import" button. Clicking the import button would reveal a dialog, where the user selects a provider (eg. CSV would be a provider - Excel could be a another provider). Clicking "CSV" would let the user upload a CSV file, and if it has the exact column names the provider is looking for, the code behind the provider attempts to add the redirects in the file.
With the same starting point, the user clicks the "Import" button, and then chooses the "CSV" provider in the dialog. Rather than just uploading the CSV file, the user will now also be prompted by a list of options relevant to the CSV provider - eg.:
- the encoding of the file
- the separator used in the file (a lot of CSV files use semicolon instead of comma)
- a dropdown for matching each redirect property to a column in the CSV file

In some cases, the options can perhaps also be auto-detected. Eg. auto-select the dropdowns if a column name matches one of the column names that could be hardcoded in the package. Eg. if a column in the CSV file is named Root ID, the package would auto-select that column for the root ID option.

For both 1) and 2), it might also be useful with a preview of the redirects in the file - eg. to check whether the file has been parsed properly, which redirects already exist etc.

If a preview feature is implemented, it would then require that we save the uploaded file for more than just the current request. How could this work?

Just store the uploaded files somewhere under /App_Data/? Or in the database? (right now I'm not sure how big a file for this could be - but we're probably only talking kilobytes at max?)
What happens to an upload file over time? Should it be automatically deleted after X days or when the import is completed? Or would it be a manual task for the user to delete the file again once done?

When an import is complete, the user should probably also be presented by some kind of result. Eg. if the import failed for one or more of the redirects from the uploaded file.

tomvanenckevort-hp commented 6 years ago

@abjerner I think it's best to start off simple, with just a CSV file that gets uploaded, parsed and imported (overwriting existing ones if that option has been chosen).

Preview feature could be useful, but you could simplify it by just doing the import and show a summary at the end of the import (i.e. "100 new redirects added, 20 redirects updated, 5 redirects failed") and have the option to show which ones have failed. The user can then fix their CSV and upload it again to add the missing redirects.

Column mapping: not really necessary in my opinion. I find that SEO teams are often very proficient in using Excel, so to create a CSV/XLSX file in a specified format is not really an issue.

Matthew-Wise commented 6 years ago

Colleague of mine has started working on this - https://github.com/EnjoyDigital/Skybrud.Umbraco.Redirects

Still a work in progress but figured I'd mention it

abjerner commented 6 years ago

@Matthew-Wise Thanks for the info. I will have a look at that ;)

I've been working on the import part a bit as well. It's mostly just at the playground state at the moment (which is why I haven't pushed it to GitHub yet - although I probably should).

It works around an IRedirectsProvider interface, which can be used for both importing and exporting redirects. A solid implementation could then be CsvRedirectsProvider, XmlRedirectsProvider or ExcelRedirectsProvider. I'll look into pushing my current work to a new branch ;)

mzajkowski commented 5 years ago

@abjerner @Matthew-Wise did anything move further in this topic in your projects? I'm once again ahead of the massive import of the huge CSV/XLS file from the SEO agency and would love to know if there is anything that might save some of the time required to parse it "manually" (= outside of the packages / Umbraco). Beers on me!

Matthew-Wise commented 5 years ago

@mzajkowski the Enjoy Digital branch has a working importer. We have it in production. I know a few people have as well. It's a little out of date so will need updating.

mzajkowski commented 5 years ago

Nice! I'll give it a go then too.

abjerner commented 5 years ago

This is unfortunately one of those things I haven't had the time to follow up on (although I probably should).

I made some progress of my own at some point, so I should make sure to push that - exactly as I wrote last year that I would 🙄

My idea was to make the import/export logic extensible so that you could make your own "provider" for formats not supported out of the box. I can't remember how much progress I actually made, but I can try to follow up on that.

I can't remember whether I've tried Enjoy Digital's fork, but I'll make sure to check up on that as well 😉

ronaldbarendse commented 4 years ago

Any progress on this? I've created a script to generate SQL inserts from a list of URLs in the meantime, but having to change all destinations (as they default to /) still requires a lot of editing afterwards...

abjerner commented 4 years ago

@ronaldbarendse I've worked a bit on it recently, but nothing finished yet. It also seems that I forgot to push my progress 😱

When I have some more time again, this is definitely something I'd like to prioritize.

hfloyd commented 3 years ago

@abjerner - Where are you with this? Did you get your code pushed? Did you decide a separate package would be best? I was thinking about helping out with this, but wouldn't want to step on any toes or waste time re-inventing the wheel...

abjerner commented 3 years ago

Hi @hfloyd. Unfortunately my past year has been very busy, which both means that I haven't really been able to work much on this feature, and I haven't been as a good at responding to issues and PRs as I feel I should be.

I'm not entirely sure where I've put the work I've done so far, so I need to have another look for that. I did find the code for some other issues I've been trying to fix, so I'll try to push some of that work.

I haven't really decided whether an import feature should be part of this package, or as a separate package. If we start with a simple import feature, and then add features over time, it would perhaps make sense to release as a separate package, as changes made to the import package then wouldn't cause breaking changes to the main package.

One important thing about an import feature is also to determine how it should work. None of our own clients have asked directly for a feature like this, so therefore I have some uncertainty about how others would use such a feature.

A first iteration could be so that the user could upload a CSV file, and the importer would then look for specific column names in the CSV file (eg. following the column names in the database table), and then import the rows as new redirects accordingly. This of course requires the CSV file to follow a very specific format, which then may not be so user friendly.

Over time I'd like to give users more control over the import. This could for instance be, so that when a user uploads a CSV file, the user is then prompted to map CSV columns to the correct properties in the C# models (or columns in the database). This adds a bit more complexity for the implementation, but would give users more freedom and flexibility.

I haven't found the right way to approach this yet, so any feedback on this would be more than welcome 😉

hfloyd commented 3 years ago

@abjerner , Thanks for your response. I think there is some useful use-case description in the previous comments here, but if I were to just consider my own experience and how I would approach it...

There are two times an import would be useful.

In a new website build, as part of the last development steps, old URLs should have redirects set up, and I think doing it in this editable way is better than using web.config redirect maps, etc.
In some organizations with well-funded marketing teams, an outside SEO company might be engaged to review analytics, etc and make recommendations about redirects. Also, an active content editing team might be checking 404 logs, etc. and want to be proactive about addressing them.

So, in use-case 1:

Probably being done by a developer, or other tech-savvy person
Generally a one-off process

Use-case 2:

Might be done by a dev, or a content editor
Might happen frequently on a regular basis

If we just wanted to get started, my inclination is to start with the simplest MVP, which involves addressing use-case 1.

MVP includes:

Reading a CSV file with predefined headers
Looping each item and importing it into the db table
Using the text of the imported "To" field to lookup a published content node with a matching url segment, and provide that node key for the redirect on import
Provide a final list of any import errors (perhaps downloadable as a CSV for easier resolution and reimport).

Since not everyone will need import functionality, and it would involve installing additional dependencies, I think a separate package would be ideal. Also, that would allow for eventual permissions issues to be handled, because any back-office dashboard UI could be secured separately from the main Redirects interface.

If I were doing this all by myself, my MVP wouldn't even have a UI. I'd just set up a webapi, assume a standard CSV format, and assume the folder that the CSV would come from. (So, not even an upload control)

In the scenario of a second package, ideally there would be a dependency on a ".core" version of the redirects package which would provide the redirect models and access to whatever the basic "insert" service functionality is. I haven't looked closely enough at the source code to determine if this is already in place, but I think having that separates from the UI specific parts would be ideal.

abjerner commented 3 years ago

Great @hfloyd. I'll try to reply back tomorrow 😉

ronaldbarendse commented 3 years ago

Maybe a simpler bulk import would be the possibility to paste multiple URLs and have it open the redirect/destination picker for each one? This is probably far easier for editors then having to craft a very specific CSV-file, especially when dealing with links to content/media (having to lookup IDs/UDIs).

Besides the manual SQL script (see https://github.com/skybrud/Skybrud.Umbraco.Redirects/issues/27#issuecomment-627422868), we've also created an XML sitemap importer that uses Examine to get the 'best matching' destination content to use as destination (or the root/site node as fallback). This basically splits the source URL into URL segments, boosts the last part (as that's the most important bit for that specific page) and compares this to content names/URL segments.

Both solutions would deal with the use-cases mentioned by @hfloyd, without having to add additional dependencies for CSV parsing and/or big changes to the UI. The 'Add redirect' button could get a dropdown for batch adding a URL list or XML-sitemap and the destination picker might need a 'Skip' button (having 'Cancel' stop the batch add and 'Submit' save the current redirect and open the next).

hfloyd commented 3 years ago

That's a really interesting idea, @ronaldbarendse ! I think having both available would be cool.

In my experience, someone has already done the work of looking at the new (front) of the website and selected the new page for the redirects - so I'd like to utilize that effort via the 2-column import. I don't think think the CSV would be too difficult for most editors to handle - I have received many of these type of files from different clients through the years. My idea would be that they would just be inputting the simple 2-column CSV like this:

Old         |   New
/someoldpage.php    |   /section/newpage

On import, the code would attempt to find the content node with a url of "/section/newpage" and use the id from that for the insert operation.

Some additional items for consideration:

stripping off TLDs from import lists
handling multiple-site installs (with multiple top-level nodes representing different sites, each with an assigned hostname and similar page structure )
handling 1-to-1 multi-lingual sites

Getting back to your "Interactive Bulk Insert" functionality, @ronaldbarendse, I think the best user experience would involve a slightly different interface...

Step 1:

Step 2:

The v7 "301 URL Tracker" included an additional feature of "Keeps track of not found (404) requests, so you can easily create redirects for them" - which could be added to this package as well, and this sort of "interactive" interface would be a great way to allow Editors to assign redirects for 404 Urls.

(@abjerner - Have you considered adding your package to the Umbraco HacktoberFest? )

hfloyd commented 3 years ago

@abjerner - Regarding my previous suggestion about creating a ".Core" package which could be used for any external "add-on packages"...

In the scenario of a second package, ideally there would be a dependency on a ".core" version of the redirects package which would provide the redirect models and access to whatever the basic "insert" service functionality is. I haven't looked closely enough at the source code to determine if this is already in place, but I think having that separates from the UI specific parts would be ideal.

I was able to break it out easily. (see the branch here) You'd just need to set up a second NuGet package which only includes the dll from that second Project. Let me know what you think.

abjerner commented 3 years ago

Thanks for feedback @hfloyd and @ronaldbarendse 👍

I like having both a way to import redirects from a CSV file (and possibly other formats in the future) as well as a way to bulk add redirects via the UI.

Based on the feedback I've received so far for an import feature, I think most users would benefit from import of CSV files, so I think that should be the first goal.

In my head, I had it that the CSV file would contain all or at least most of the columns also found in the database table. Having just Old and New could be a good approach for a MVP, and then we can iterate on it from there. Handling multi sites, multi-lingual sites and so on could be part of a second iteration.

The interactive import also seem interesting, but I think we should skip that for the first iteration, and then follow up later. I like @hfloyd's screenshot though 👍

Current package vs new package Specifically for the import functionally, I think it would best to create an add-on package for now, which then extends the current redirects package. This way I fell like we can better improve it over some iterations, and then perhaps at some point, put it back into the main package.

If we add something to the main package now, it may be harder to change it later, as people may start depending on the initial logic. So if we were to make some breaking changes, I'd rather to that in a separate package. Some of our packages are marked as either alpha or beta, which means that it's likely there will be breaking changes.

404 tracker Tracking of 404 errors has it's own list of problem. We have had some heavy traffic sites in the past that resulted in several millions of rows, which then also resulted in some performance issues when trying to access these from either the frontend or the backend. I'd rather have this as a separate package. And as it's not something I or Skybrud will have much need for, I think it would be best to create this as a new community package instead.

Core package Having to maintain both a core package and resources/UI package creates some extra work. While some of the work can probably be automatized, I'd rather keep the current package as a single package like now. There are other ways to handle the resources if they should be a problem. I hope that makes sense 😉

Hacktoberfest I haven't added any of our packages to Hacktoberfest as I wasn't sure I had the time to handle any incoming PRs. I'm still behind on existing issues and PRs. But if you do make a PRs to this or other of our packages, I'll make sure to add the necessary labels to the PR to make it count for Hacktoberfest 🎉

hfloyd commented 3 years ago

Thanks, @abjerner ! (I'm glad you liked the little mockup ;-) )

RE:

In my head, I had it that the CSV file would contain all or at least most of the columns also found in the database table. Having just Old and New could be a good approach for a MVP, and then we can iterate on it from there. Handling multi sites, multi-lingual sites and so on could be part of a second iteration.

Perhaps we could just utilize the current "add redirects" options - Site, Type, Query string, and have those apply to all the imported urls.

abjerner commented 3 years ago

@hfloyd yes, something along those lines 👍

Although there might be situations where redirects would have different options (but support for that could wait to a future iteration).

In case you want to work on this, I've set up a new repo for an import package here: https://github.com/skybrud/Skybrud.Umbraco.Redirects.Import

The solution doesn't really contain much code yet, but I added a JavaScript file with the following:

app.run(function (eventsService) {

    eventsService.on("skybrud.umbraco.redirects.dashboard.init", function (_, args) {

        const add = args.buttonGroups.find(x => x.alias === "add");
        if (!add) return;

        add.subButtons.push({
            label: "Import",
            labelKey: "redirects_import",
            handler: function() {
                // open import dialog
            }
        });

        add.subButtons.push({
            label: "Export",
            labelKey: "redirects_export",
            handler: function () {
                // open export dialog
            }
        });

    });

});

The file utilizes a new change I made in the main package (not released yet), so other packages are now able to control the buttons in the dashboard. For instance here, I'm changing the Add new redirect-button to a dropdown, so if you click the caret, you'll get both an Import and an Export option.

The two buttons in the dropdown doesn't really do anything yet, but the idea would then be, that they oven an overlay/dialog for for that particular action.

abjerner commented 3 years ago

I tried dusting off Total Commander (it's really amazing for searching through file contents), and found a ZIP I had taken as a backup of my work.

I can now choose to import a CSV file from my disk, and the importer tries to analyze the CSV file, so it will detect a number of column aliases - eg. as:

        private void MapCsvColumns(CsvInternalImportOptions options) {

            foreach (CsvColumn column in options.File.Columns) {

                switch (column.Name.Replace(" ", "").ToLowerInvariant()) {

                    case "rootnodeid":
                    case "domain":
                        if (options.ColumnRootNodeId != null) break;
                        options.ColumnRootNodeId = column;
                        break;

                    case "url":
                    case "inboundurl":
                        if (options.ColumnInboundUrl != null) break;
                        options.ColumnInboundUrl = column;
                        break;

                    case "linkid":
                    case "destinationid":
                        if (options.ColumnDestinationId != null) break;
                        options.ColumnDestinationId = column;
                        break;

                    case "linktype":
                    case "linkmode":
                    case "destinationtype":
                    case "destinationmode":
                        if (options.ColumnDestinationType != null) break;
                        options.ColumnDestinationType = column;
                        break;

                    case "linkurl":
                    case "destinationurl":
                        if (options.ColumnDestinationUrl != null) break;
                        options.ColumnDestinationUrl = column;
                        break;

                }

            }

            if (options.ColumnRootNodeId == null) throw new RedirectsException("No column found for *root node ID*");
            if (options.ColumnInboundUrl == null) throw new RedirectsException("No column found for *inbound URL*");
            if (options.ColumnDestinationId == null) throw new RedirectsException("No column found for *destination ID*");
            if (options.ColumnDestinationType == null) throw new RedirectsException("No column found for *destination type*");
            if (options.ColumnDestinationUrl == null) throw new RedirectsException("No column found for *destination URL*");

        }

It still isn't a finished import, so for now, it can do the following:

redirects_import

My CSV file looks like:

"Domain";"Inbound URL";"Destination ID";"Destination Type";"Destination URL"
"redirects.omgbacon.dk";"/hest";0;"url";"/test"
"redirects.omgbacon.dk";"/rød-grød-med-fløde";0;"url";"/roed-groed-med-floede"
"redirects.omgbacon.dk";"/about";1110;"content";"/about-uuuuuuus"

The imported doesn't actually import the redirects yet, but the WebAPI endpoint returns the options that would be used for the import. So the two first redirects, which doesn't specify a destination ID, are mapped "as is".

The third redirect mentions a specific destination ID. I think it was the About us page when I created the CSV file, but now that ID instead corresponds to the page about Jeroen Breuer (from the starter kit). So the destination URL is mapped to /people/jeroen-breuer/ instead of /about-uuuuuuus from the CSV file.

Keys/GUIDs haven't really been handled well in the past. I pushed a new release last week to fix this in the main redirects package, but as my import code predates this release, I also need to fix it in the import code.

The idea I had about the importer when I last worked on this, was that you can define a redirects provider (might need a better name going forward), so for instance the package would start out with a CSV redirects provider:

    public class CsvRedirectsProvider : IRedirectsProvider {

        #region Properties

        /// <summary>
        /// Gets the name of the provider.
        /// </summary>
        public string Name => "CSV";

        /// <summary>
        /// Gets the description of the provider.
        /// </summary>
        public string Description => "Lets you import from and export to CSV files.";

        /// <summary>
        /// Gets whether this provider supports importing redirects.
        /// </summary>
        public bool CanImport => true;

        /// <summary>
        /// Gets whether this provider supports exporting redirects.
        /// </summary>
        public bool CanExport => true;

   }

A provider describes whether it can import and export redirects. Using this concept of providers would then also enable developers to create own provider - eg. for Excel files (XLS/XLSX) instead of CSV, or another provider for an XML file with it's own custom logic as Ronald describes.

I'll try to have another look at this tomorrow, or at least make sure to commit my work this time 😊

hfloyd commented 3 years ago

Hi @abjerner

I had some time last week, so I figured I'd take a crack at the Import code, as an unaffiliated package. My current iteration allows you to select a file already in a specified folder, set options, and do the import successfully. I used the "LinqToCsv" NuGet package to handle the CSV conversion - I've used it in other projects before and it is very simple to set up using a Model with attributes. I didn't spend any time on the Angular/back-office UI, rather using my go-to Razor-provided WebAPI UI setup. (Obviously, a back-office integration is more elegant.)

Basically, the import works as follows:

If the "new" url is fully-qualified (aka "https...." rather than "/mypage"), assume it is a Url DestinationType, so just use that.
Otherwise, try to match the New url to a Content Url, if matched, use that (Id, GUID, etc), otherwise, look for a matching Media url. If nothing found, return as an error.

It looks like you have most of the UI set up... If you want to take a peek/test drive my current code, perhaps there is a way I can integrate some of my back-end "importing" work into your new package and we can have something fully-functional. (The pertinent part of the import logic is here.) I'd love to get your initial feedback on that.

Ps. This is very cool:

The file utilizes a new change I made in the main package (not released yet), so other packages are now able to control the buttons in the dashboard. For instance here, I'm changing the Add new redirect-button to a dropdown, so if you click the caret, you'll get both an Import and an Export option.

RE:

The idea I had about the importer when I last worked on this, was that you can define a redirects provider (might need a better name going forward),

I think everything associated with this separate tool/package should be named along the lines of "Importer" or "RedirectImporter" (to differentiate from the "redirecting" functionality)- so perhaps "RedirectsImportProvider" - and then you could have CSV, Excel, XML, whatever...

abjerner commented 3 years ago

Hi @hfloyd,

I wasn't sure whether you had started working on this, so I also did some work over the weekend.

So for the Skybrud.Umbraco.Redirects.Import, I used my own Skybrud.Csv. I didn't know about LinqToCsv - it only has a few more downloads than mine 🙄

I wrote most of the C# logic a while ago, but worked over the weekend on integrating it with the backoffice and Angular. It's still work in progress, but it now supports the following:

Select the redirects provider I started using the term redirects provider a while ago in lack of something better. I agree it should be something like CsvImporter or CsvRedirectsImporter. *Importer might still be a viable name if an implementation should only support exporting, and not importing (not sure when that will be the case, but my interface allows for that).

Configure options for the import Each provider/importer defines a list of config fields, and the values of those are then posted along to the import API:

Importing the redirects Like I described earlier, the code for the CSV import known a few different column names, and will try to look for those.

For instance it will look for either a column and Root Node ID or Domain. If the value is a numeric ID, it will be treated as the root node, but other values will be treated as the domain, which it will then look up in Umbraco to find correct root node ID automatically.

Right now it will look for five types of columns, and will throw an exception of it doesn't find those in the CSV file. Going forward it should probably be less restrictive about these columns. Eg. if a Root Node ID or Domain column isn't specified, it will result in a global redirect instead. Or there could be an additional step after the CSV file has been uploaded, where the user can specify this.

Right now the code will actively try to guess the encoding of the CSV file - unless the user select an encoding explicitly in the UI. I've also added an option for the separator, but I haven't implemented this yet - so default is now semi colon.

Showing the result of the import I haven't really worked much on this part yet, so for now I only have a <pre> element with a big JSON blog representing the imported redirects. I like your way of showing failed and successful redirects, so if we can wrap that up in Umbraco's components instead, I think we should go for something like that 👍

While it may only be under some specific conditions, it does allow you do an import 😄

I haven't look much at your import controller yet, but I'll try do find some time do that 😉

hfloyd commented 3 years ago

@abjerner - You have a lot more csv-specific import options in your version - very nice work.

In general (based on the excel lists of Redirects I have gotten from clients), the people compiling the redirects list often don't have access to, or know anything about the CMS, so they would never be providing any Umbraco Node IDs or GUIDs. It's possible that they could perhaps provide a "destination domain" column - but based on the files I've been handed, if anything, the "New url" column would just include the whole url (including domain). In my "bare-bones' version, I am requiring that the CSV be properly formatted - excluding the domain from the "New Url" data (unless it is off-site). I could perhaps test the domain in the New Url column - and see if it matches one of the known domains for the Umbraco installation, and if not, assume it is off-site.

RE:

Showing the result of the import I haven't really worked much on this part yet, so for now I only have a <pre> element with a big JSON blog representing the imported redirects. I like your way of showing failed and successful redirects, so if we can wrap that up in Umbraco's components instead, I think we should go for something like that 👍

I was thinking of actually adding an "export" option to the failed list, so they can make corrections on just those and attempt to re-import them. What exactly do you mean by "Umbraco's components" - are you referring to the way data needs to be returned for back-office display, or something else?

hfloyd commented 3 years ago

@abjerner Would you have any objection to setting this up with a Service architecture? I think that might make it easier for us to both share code in a way which can evolve over time.

abjerner commented 3 years ago

Thanks for the feedback @hfloyd 👍

My idea was that the importer would be able to handle both scenarios - that is, both the simple CSV file with columns for the old and new URLs, but also the more advanced CSV files with more information about how to add the redirects. And then also the scenarios in between. Everything doesn't have to work from the beginning - we can improve the importer over more iterations.

An export feature for the failed redirects makes good sense. I think I also started out on a general export feature somewhere, so if I can find that, maybe we can reuse some of that here.

By Umbraco's components, I'm referring to using the same classes (or directives) as Umbraco does, so the UI will look consistent. Not necessarily a requirement for the first iteration, but I think it should be in the future.

I'm not sure what you are referring to with setting up a service architecture? Can you elaborate on this?

hfloyd commented 3 years ago

@abjerner - RE:

By Umbraco's components, I'm referring to using the same classes (or directives) as Umbraco does, so the UI will look consistent. Not necessarily a requirement for the first iteration, but I think it should be in the future.

Sure, that makes the most sense. My own project UI is totally outside of the back-office, and anything related to the UI (other than perhaps general "wireframe" sort of ideas) wouldn't really be directly usable in this more integrated setup.

RE:

I'm not sure what you are referring to with setting up a service architecture? Can you elaborate on this?

I just meant that we could consider the bulk of the processing code being set up in a "Service" to keep it all together, regardless of the API controllers needed to make the UI function.

So...usage in the APIs would be something like:

var pathToUploadedFile = ...; IRedirectsImportExportProvider myCsvProvider = .....; var importerService = new RedirectsImportExportService(); var results = importerService.ImportFile(pathToUploadedFile, myCsvProvider);

//do stuff with UI... var sucesses= results.ImportedRedirects; var failures = results.ImportErrors;

We could then add whatever additional features as desired, like: Export(IEnumerable ImportErrors, IRedirectsImportExportProvider Provider) etc.

abjerner commented 3 years ago

Yes, as much logic as possible should be located in the service (or the individual provider/import implementations if more specific).

I already tried to minimize the logic in the controller (or am I missing something?):

https://github.com/skybrud/Skybrud.Umbraco.Redirects.Import/blob/v1/latest/src/Skybrud.Umbraco.Redirects.Import/Controllers/RedirectsImportController.cs

The service however ought to be injected via dependency injection. The code is still a bit messy here and there from me playing around (most of it was written for V7).

hfloyd commented 3 years ago

Let me pull everything down and take a closer look.

RE:

The service however ought to be injected via dependency injection. The code is still a bit messy here and there from me playing around (most of it was written for V7).

That's fine with me. I'm a bit new to DI, so feel free to change things or advise on better handling...

abjerner commented 3 years ago

Don't worry about the DI part. My comment regarding DI was about my own code 😉 (it could also be addressed in a later iteration)

hfloyd commented 3 years ago

Well, in any case, I think we can try our best to make it as version-eight-like as possible :-)

hfloyd commented 3 years ago

Okay, now that I've actually forked and pulled your new repo, I can see why you were confused about my comment about a Service - since I see you have a Services folder 🤣

So, my follow-up question: I noticed that the return types for the Service are all "object" - is that just because it is getting used directly via the JS Controllers? Is there a way the Service can use other sorts of Models? Perhaps via a linking Web API that returns the "object"s?

Mostly I ask because I am most comfortable working with strongly-typed things, and they give certain advantages, like compilation checking, intellisense, etc. Also, it would then mean that the Service could technically be available for usage beyond just the back-office UI - think, for example, a case where a dev could install the Importer package, and then set up automated scheduled importing from an FTP folder, or using 404 logs or who knows what other expanding fun!

abjerner commented 3 years ago

@hfloyd yes, your right - the service should have a return type for the various methods. And exactly for the reasons you're describing.

Again, the code is a little messy. For the code I did a while ago, I hadn't really decided what the methods should return. I played around with that over the weekend, but haven't set a proper return type yet.

For now, I think it should be List<RedirectImportItem> for the Import methods as that is what the methods are actually returning. You're welcome to submit a PR for that - otherwise I'll fix when I'm working on the code again 😉

Instead of List<RedirectImportItem> as the methods are returning now, one could perhaps argue that the methods should return an object/type representing the overall import result instead. Eg:

public class RedirectsImportResult {

    public List<RedirectImportItem> Items { get; }

}

or:

public class RedirectsImportResult {

    public List<RedirectImportItem> Success { get; }

    public List<RedirectImportItem> Failed { get; }

}

hfloyd commented 3 years ago

Actually, i was just working on that! - It's not committed yet, because I swiped some stuff from my other project and I'm getting it integrated first... but here's a preview:

public class ImportResultSet
    {
        public DefaultImportOptions DefaultOptions { get; set; }
        public string Filename { get; set; }
        public List<RedirectItem> SuccessItems { get; set; }
        public List<ImportErrorItem> ErrorItems { get; set; }
        public bool HasErrors { get; set; }
        public List<string> ErrorMessages { get; set; }

        public ImportResultSet()
        {
            SuccessItems = new List<RedirectItem>();
            ErrorItems = new List<ImportErrorItem>();
            ErrorMessages = new List<string>();
        }
    }

Also, I think the specific implementations of IRedirectsImportExportProvider should be responsible for just converting the import format (whatever that may be - CSV, Excel, XML, JSON...) into a standardized model that the main Importer Service can then use to lookup nodes, update the DB table, etc. I think that will keep the "heavy-lifting" separate from the Providers - which then need to only handle one thing. That will also shield the Providers from any internal structural changes made to the main Redirects functionality (if, for instance, the DB table schema was changed, etc.)

hfloyd commented 3 years ago

@abjerner : Okay, I've pushed up my current code for you here: https://github.com/skybrud/Skybrud.Umbraco.Redirects.Import/pull/2

Do you think we should move this conversation over to the new Repo?

mzajkowski commented 3 years ago

Just remembered myself about this feature and thread while I deleted a custom build from a fork for one of the clients. Any plans / actions around this? Still something what I think we all need at least for any new builds/migrations.

abjerner commented 3 years ago

An import feature is the most requested feature for this package, and it's also something that I believe this package should have. But to be honest, I've been overworked for a while now, and haven't really had the energy to work on this for some time.

I did make some progress back in October, and Heather also made a PR with her work so far, but I haven't had the energy to look at this yet (sorry Heather). I hope I'll be able to do that soon, but as things are now, I can't make any promises for when this will be.

abjerner commented 1 year ago

Although still in alpha, this is now possible through this add-on: https://github.com/skybrud/Skybrud.Umbraco.Redirects.Import

skybrud / Skybrud.Umbraco.Redirects

Feature request: bulk import #27