search

silverstripe / silverstripe-redirectedurls

Silverstripe module to let users to configure arbitrary redirections in the CMS
BSD 3-Clause "New" or "Revised" License
32 stars 48 forks source link

Export contains all database fields #71

Open robingram opened 5 years ago

robingram commented 5 years ago

Just a question really. In the SS4 version of this module the ModelAdmin overrides getExportFields and queries the database schema to get the field list: DataObject::getSchema()->databaseFields($this->modelClass). This returns all fields including ID, LastEdited and Created.

Is this the intended behaviour? Previously it used singleton($this->modelClass)->db() and so only exported three fields.

robingram commented 5 years ago

This is actually more serious than I first thought. Because the first column is ID this triggers a bug in Excel:

Export a gridfields as CSV. Open in Excel, edit one of the fields and re-save. Try to open the file again in Excel. This results in an error message because of a file type mismatch. Excel thinks it is a SYLK file now. If you open it in a text editor you'll find that it is now tab separated and obviously won't import properly.

robingram commented 5 years ago

Also applies to other modules, including silverstripe-registry.

robingram commented 5 years ago

Note also that the exported CSV no longer matches the specification that is displayed in the import dialogue, unless the additional columns are removed.

dhensby commented 5 years ago

@robingram are you prepared to open a PR to address this?

The excel issue with opening CSVs that have a primary column called ID is a "known issue" in excel, not a lot we can do about that other than tell people to open their CSVs via the "import CSV" wizard

robingram commented 5 years ago

@dhensby Can do but I'd prefer to address it on silverstripe-registry first as that's more urgent for us.

What's the recommendation? Two options that come to mind are set it back to what it was before, assuming that singleton($this->modelClass)->db() is still valid, or to remove the override altogether so it falls back to summary_fields. The latter allows for the fields to be overridden but might be problematic for the duplicate check in this module.

robingram commented 5 years ago

...Though mapping summary fields in the same way that it does for databaseFields would solve that by making sure that the headings match the field name rather than the header text. The problem would come if someone took FromBase out summary_fields.

robingram commented 5 years ago

A PR has gone into silverstripe-registry based on summary_fields but I'm not sure how to proceed with this module. If we use summary_fields then it could potentially be overridden to remove FromBase and then the duplicate check of the importer will throw an error. I'd argue that it's a field that's pretty fundamental to the module though so maybe that risk is small.

We could ensure that FromBase is always in the export field list but that seems sneaky.

Thoughts anyone?

dhensby commented 5 years ago

A PR has gone into silverstripe-registry

link?

robingram commented 5 years ago

https://github.com/silverstripe/silverstripe-registry/pull/51

Robbie merged it.