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

Omit casting on a per attribute basis - open to a PR? #635

Open
ultrono opened this issue Mar 1, 2021 · 6 comments
Open

Omit casting on a per attribute basis - open to a PR? #635

ultrono opened this issue Mar 1, 2021 · 6 comments
Assignees
Labels
cannot reproduce Unable to replicate the issue question

Comments

@ultrono
Copy link

ultrono commented Mar 1, 2021

Hi,

Would the package maintainer be open to a PR for the following?

Allow an array of attributes that shouldn't be cast, to be specified on a per model basis?

From http://www.laravel-auditing.com/docs/9.0/audit-transformation I can see here a new key role_name has been specified, but it seems odd to introduce keys outside the scope of your model.

Example, assume I have a task model with a status_id column. status_id is cast to an integer.

On the task model the transformAudit() method has been specified as follows:

public function transformAudit(array $data): array
{
    if (Arr::has($data, 'new_values.status_id')) {
        $data['old_values']['status_id'] = optional(Status::query()->find($this->getOriginal('status_id')))->title;
        $data['new_values']['status_id'] = Status::query()->find($this->getAttribute('status_id'))->title;
     }
     
     return $data;
}

Note here, as the documentation suggests I could simply specify a different key, other than status_id. However, doing so causes both keys to be present in the database old and new columns. To only store the new key I have to omit the old key i.e.

public function transformAudit(array $data): array
{
    if (Arr::has($data, 'new_values.status_id')) {
        $data['old_values']['status'] = optional(Status::query()->find($this->getOriginal('status_id')))->title;
        $data['new_values']['status'] = Status::query()->find($this->getAttribute('status_id'))->title;
        unset($data['old_values']['status_id'], $data['new_values']['status_id']);
     }
     
     return $data;
}

Instead of storing the old an new status_id integers, the status title is being stored as a string.

Within the audits table:

// old values
{"status_id":"Awaiting Feedback"}

// new values
{"status_id":"Current Jobs"}

As this package now honours Eloquent casts, within my auditable view, the old an new status_id values are always zero - as expected due to the model casts.

I could remove the casts within the tasks model. However, this is an existing project that makes use of strict comparisons.

I'd propose a very simple PR to accommodate this.

Within the package Audit model, adjust the getDataValue method to account for an excluded attribute. If excluded, simply the return the raw value:

/**
   * {@inheritdoc}
   */
public function getDataValue(string $key)
{
    if (!array_key_exists($key, $this->data)) {
       return;
     }

     $value = $this->data[$key];

     // if the attribute should not be casts, simple return the raw value
     if (in_array($key, $this->excludedAuditableAttributeCats, true) {
       return $value;
     }

     ...

Would this be something the package owner would consider?

Thanks

@ale1981
Copy link

ale1981 commented May 25, 2022

This is over a year old but no comments, I definitely think this would benefit the package.

@jasonfish568
Copy link

Not only do I have the same requirements, but also the current version has a bug on this. After transforming the data, it doest get stored correctly. However, when retrieving getModified(), it doesn't return the data from the audit table, it fetch the current information from the database. Meaning the status we transformed before won't be even showing when you use getModified to show the audit data.

@ultrono
Copy link
Author

ultrono commented Mar 13, 2023

Had completely forgotten about this. Since I raised this I've unfortunately been using the unset approach above. Not end of the world 😮‍💨

@MortenDHansen MortenDHansen self-assigned this Mar 16, 2023
@MortenDHansen MortenDHansen added the v14 Considered for v14 label Mar 16, 2023
@MortenDHansen
Copy link
Contributor

We are working on a solution, but have to be careful with regards to backwards compatibility.

Related to this: #799

@MortenDHansen
Copy link
Contributor

Right. To get back to this, i am sorry to say i don't understand what the desired outcome is.

If possible, please outline what the desired behaviour is, or describe how to reproduce the problem.

@MortenDHansen MortenDHansen added question cannot reproduce Unable to replicate the issue and removed v14 Considered for v14 labels Mar 30, 2023
@radcloudcorp
Copy link

Here I have the same behavior reported above. I have the pat_sex field in the application but I wanted to change it in the log to sex. However, when I do the transformation, both keys are saved, which is not necessary because the old value must be discarded.

Screenshot 2024-06-07 at 08 57 56

This way the two values ​​are new and old values

Screenshot 2024-06-07 at 08 58 23

To get around this, you have to use your friend's tip by removing the old key with the unset for each field that is transformed. It would be great if something native to the package was capable of performing this task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cannot reproduce Unable to replicate the issue question
Projects
None yet
Development

No branches or pull requests

5 participants