OBDS-129 Add import trigger for location groups, locations, organizat…

[awalkowiak]

…ions, bin locations, categories and products with GET /api/loadData

[jmiranda]

Where did this data come from?

[jmiranda]

What is the reason for disabling this?

[awalkowiak]

@jmiranda this was a suggestion for development purposes. This is the WIP version, and @hdsldv will remove it in the final version

[ghost]

reverted

[jmiranda]

@awalkowiak @hdsldv no worries. i was just wondering if there was some kind of conflict with the changes we were making.

A better approach for local development environments would be to create ~/.grails/openboxes-config.groovy and add the property there.

[jmiranda]

Can we convert this to Groovy DSL syntax

openboxes.configuration.data { 
    locations { 
        enabled = true
        url = 
    }
    ...
}

[awalkowiak]

Yup, agree. I gave the same suggestion.

[ghost]

done

[ghost]

I reverted change to DSL as I was not able to use it as key-value map

[jmiranda]

Sorry I screwed that up. It should probably look something like this.

openboxes { 
    configuration { 
        data { 
            locations { 
                enabled = true 
                url = "<whatever>"
            } 
            ...
        } 
    } 
}

[ghost]

It works, thanks

[jmiranda]

Let's make this more REST-like And we need to do some thinking about naming convention here.

Something like this maybe

GET /api/config/data/demo

But I'll do some more thinking about this tomorrow.

[ghost]

For now changed to /api/config/data/demo

[jmiranda]

Move this to a new ProductCatalogService.

[ghost]

done

[jmiranda]

I think these methods should go into DataService.

[ghost]

I do not think it is possible. LoadDataService requires productService, which requires dataService. After moving to DataService there is creating bean exception, because of circular reference between DataService and ProductService

[ghost]

Where did this data come from?

@jmiranda If you have csv files in mind, they are linked in ticket description (except categories which is copy of UNSPSC_categories.csv). I converted them from excel to csv format

[jmiranda]

Oh yeah this doesn't feel right. I don't know if we want to be setting a password at all. I think we might need to do some type of activation here. But we should have a discussion with Kelsey about expected behaviors. This could be a huge security hole.

[ghost]

I added active: false for imported users as password has blank: false constraint

[jmiranda]

This should be already handled through the unique constraint on the User domain.

[jmiranda]

You just need to check if the save was successful and throw an exception if not.

// The hasErrors checks whether there were errors when data binding
if (user.hasErrors() || !user.save()) { 
    throw new ValidationException("invalid user", user.errors)
}

Or you can explcitly validate the domain before saving.

user.validate() 
if (user.hasErrors() || !user.save()) { 
    throw new ValidationException("invalid user", user.errors)
}

I prefer the first approach.

[ghost]

Added failOnError when saving user

[jmiranda]

You should use roleType.name() instead of roleType.toString() to prevent any weirdness with the implicit toString().

https://stackoverflow.com/questions/6667243/using-enum-values-as-string-literals

However, I don't think we want to be creating new system roles during import unless it's explicitly stated in the requirements. I'd rather throw an exception stating that a role with role type doesn't exist. There's also a case where one role type might be mapped to multiple roles. Not sure how we want to handle that. Probably throw an exception for that as well.

Note, once you add a role to the userRoles set, nothing else happens so all of this role / user role logic is basically a no-op as the user roles won't be persisted to the database.

Instead, you probably want to use user.addToUserRoles().

for (RoleType roleType in roleTypes) {
    Role role = Role.findByRoleType(roleType)
    if (!role) throw new IllegalArgumentException("Role for role type ${roleType.name()} does not exist")
    user.addToUserRoles(new UserRole(user: user, role: role))
}

One caveat is that we probably need to save the user before we process the roles.

[ghost]

Now null role throws exception. I'm not sure what you mean by no-op - I'm assigning userRoles set to roles property and it works just fine - user and corresponding user_role records are created

[awalkowiak]

Since the majority of work was done and only fixes to the product supplier are the remaining part to be done here, I am gonna merge this one to unblock other work. I'll create a ticket for improvements and put it in the next sprint.

Side note, the issues related to the product supplier import might be related to #3049

cc @jmiranda @hdsldv