Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not stringify object rows, skip over them instead #30

Open
dextermb opened this issue Nov 26, 2018 · 6 comments
Open

Do not stringify object rows, skip over them instead #30

dextermb opened this issue Nov 26, 2018 · 6 comments

Comments

@dextermb
Copy link
Contributor

Currently within the source object/array get stringified. I propose mapping these to null and then filtering columns in render.

Original:

    render() {
        const { row, fields } = this.props;

        return (
            <tr onClick={() => this.handleOnClick(row)}>
                { this.renderCheckboxCell(row.id) }
                { fields.map(field => this.renderCell(field, row)) }
                { this.renderButtons(row) }
            </tr>
        );
    }

    // ...

    renderCell(field, row) {
        let value = row[field.name];

        value = this.props.dataItemManipulator(field.name, value);

        if (typeof value === 'object' || typeof value === 'array') {
            value = JSON.stringify(value);
        }

        return (
            <td key={`${row.id}_${field.name}`}>{ value }</td>
        );
    }

New:

    render() {
        const { row, fields } = this.props;

        return (
            <tr onClick={() => this.handleOnClick(row)}>
                { this.renderCheckboxCell(row.id) }
                { fields.map(field => this.renderCell(field, row)).filter(field => field) }
                { this.renderButtons(row) }
            </tr>
        );
    }

    // ...

    renderCell(field, row) {
        let value = row[field.name];

        value = this.props.dataItemManipulator(field.name, value);

        if (typeof value === 'object' || typeof value === 'array') {
            return null;
        }

        return (
            <td key={`${row.id}_${field.name}`}>{ value }</td>
        );
    }

Thoughts?

@DivineOmega
Copy link
Contributor

Some advantages of the current behaviour is that it is obvious what is present within the field, and makes it easier to implement a DataItemManipulator callback.

A possibly disadvantage @dextermb brought up is that if a relationship is added to a model, its content, which could contain private information, would be immediately rendered in the table.

Ideally, we should output something that makes it obvious the column is not empty, but does not reveal private information. Perhaps we could blank out all values, or replace their content with asterisks, leaving the keys intact, then stringify as is?

@dextermb
Copy link
Contributor Author

Some advantages of the current behaviour is that it is obvious what is present within the field, and makes it easier to implement a DataItemManipulator callback.

A possibly disadvantage @dextermb brought up is that if a relationship is added to a model, its content, which could contain private information, would be immediately rendered in the table.

Ideally, we should output something that makes it obvious the column is not empty, but does not reveal private information. Perhaps we could blank out all values, or replace their content with asterisks, leaving the keys intact, then stringify as is?

Perhaps we go through and make the values undefined or null. Maybe even <redacted>.

⚠️ Although it is worth mentioning that they would be able to find the data through window.<object> or in the network tab.

@dextermb
Copy link
Contributor Author

Extra thoughts, @NilesB @JacobBrassington @wheatleyjj @ash123456789

@JacobBrassington
Copy link
Contributor

Shouldn't data that should be redacted never make it to the front end?

@dextermb
Copy link
Contributor Author

Shouldn't data that should be redacted never make it to the front end?

This issue is, that if someone were to use with on a Laravel model, then the relationship would appear everywhere and magically appear on the frontend.

@DivineOmega
Copy link
Contributor

@JacobBrassington You're very right, it shouldn't. The functionality being discussed is just an extra fail safe to ensure that even if it does somehow reach the frontend, it is not immediately displayed to casual users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants