feat: load default config when creating a project

[tomasciccola]

this should close #402

[tomasciccola]

Working on this I found that I had an error on the default config that is on the project. So - despite being out of scope - I'll probably add a test for the default config...

[EvanHahn]

LGTM!

[EvanHahn]

Nit: should this resolve relative to __dirname?

[tomasciccola]

you're right. But we can't use __dirname with ES modules, so I think need to
new URL(configPath, import.meta.url).pathname
wdyt?

[EvanHahn]

Oh yeah, that's better. I'd love that!

[EvanHahn]

LGTM assuming CI passes.

[tomasciccola]

LGTM assuming CI passes.

Yeah, I see that E2E tests are failing on project-crud.js. Gotta see whats that all about but its hard to pinpoint since the output is kinda huge - and at a first glance it doesn't seem to be related to this??? - ...

[tomasciccola]

So, tests are failing on test-e2e/manager-basic.js because expectations about how a project looks after creation changed, basically because when we create a project we implicitly load the default config. So now theres a bunch of assertions like so:

st.alike(project, {name: 'project1', defaultPresets: undefined})

which fails because defaultPresets is not undefined anymore.
One option is to change the assertion to:

st.is(project.name, 'project1')

Another option would not to run importConfig when creating a project implicitly.
A third option would be change the assertion to contemplate the loaded presets
@gmaclennan @EvanHahn wdyt??

[gmaclennan]

I think this test is the best place to check that the config has been imported when a project is created. I suggest:

• read the config in the test
• check defaults are correct after creating the project
• check fields and presets in the project match what is in the config file
• check icons are there too.

[tomasciccola]

I think this test is the best place to check that the config has been imported when a project is created. I suggest:

* read the config in the test

* check defaults are correct after creating the project

* check fields and presets in the project match what is in the config file

* check icons are there too.

the config is already read when doing manager.createProject. Do you think we should also import and call readConfig with the path to the default config and compare it to the config loaded when creating the project??
I think calling readConfig in the tests and then comparing with project.{defaultSettings,presets,icons,fields} makes sense since, if not, we're basically repeating the tests of importConfig

[gmaclennan]

the config is already read when doing manager.createProject. Do you think we should also import and call readConfig with the path to the default config and compare it to the config loaded when creating the project??
I think calling readConfig in the tests and then comparing with project.{defaultSettings,presets,icons,fields} makes sense since, if not, we're basically repeating the tests of importConfig

As far as I can see, we don't yet have any tests for project.importConfig(). What we have tests for are unit tests for readConfig. I think we need two e2e tests:

1. If you create a new project, then const { defaults } = await project.getProjectSettings() and presets.getMany() and fields.getMany() should all return the expected data in the default config - for which you need to read the config in the test to get the expected data (which is ok because you are unit testing readConfig to more-or-less make sure it's working correctly).
2. If you project.importConfig(newConfig) then you need to test the same things, but updated with the new config data.

Should probably also check icons are correctly there in both scenarios too. A test helper function to get expectedConfig might be helpful, to DRY iterating through the config reader, at the expense of memory use, which is ok in tests I think.

[tomasciccola]

this asssertion - and the following in this group - are failing. From what I can seem it seems that calling project1.preset.getMany() returns the presets corresponding to the config that I load later (completeConfig.zip instead of defaultconfig.mapeoconfig), which is weird.

[tomasciccola]

It seems that is related to assertions running in an unexpected order and solved in the last commit

[tomasciccola]

from what I can gather looking at the logs, it seems that there are a bunch of tests failing given that now there are a bunch new records loaded from the default config.
So working on that is gonna take a while...
@gmaclennan @EvanHahn does it makes sense to fix those tests here, or make another PR that fixes this directly?

[EvanHahn]

Sorry I'm just now reviewing this!

does it makes sense to fix those tests here, or make another PR that fixes this directly?

I think we should fix the tests in this PR. I like Gregor's idea of a test helper function.

Does that seem reasonable?

[tomasciccola]

Sorry I'm just now reviewing this!

does it makes sense to fix those tests here, or make another PR that fixes this directly?

I think we should fix the tests in this PR. I like Gregor's idea of a test helper function.

Does that seem reasonable?

yeah yeah, I've already created that. I think it may be just a matter of updating expected assertions by merging the current expected outcome with the default config records. That's at least what I'll try to do.

[tomasciccola]

After a short sync with gregor, we decided to add a defaultConfigPath opt to mapeo-manager. If no path is passed, we skip loading the config. This allows us to decide if a default config should be loaded and where should it live, which removes overhead from the tests.