statamic / eloquent-driver

Provides support for storing your Statamic data in a database, rather than flat files.
https://statamic.dev/tips/storing-content-in-a-database
MIT License
104 stars 74 forks source link

prevent crated_at being null while submitting a from submission #60

Closed enespolat24 closed 1 year ago

enespolat24 commented 2 years ago

Hi , i was tinkering with form submission then i noticed that created_at being null. Can you review my changes to see if it fixes the problem. thank you

image
enespolat24 commented 2 years ago

thank you for your help @ryanmitchell

enespolat24 commented 1 year ago

this pr should fix #61

andrasvati commented 1 year ago

Hi, beside the modifications you've made I'd suggest to make the firstOrNew attribute based on the id, not on form_id and timestamp, to prevent parallel submissions with the same timestamp to overwrite each other. `public function toModel() { $class = app('statamic.eloquent.forms.submission_model'); $timestamp = (new $class)->fromDateTime($this->date());

    $model = $class::findOrNew($this->id());
    return (!empty($model->id)) ? $model->fill([
        'data' => $this->data,
    ]) : $model->fill([
        'data' => $this->data,
        'form_id' => $this->form->model()->id,
        'created_at' => $timestamp,
    ]);
}`
okaufmann commented 1 year ago

This breaks all made submissions when they are shown in statamic and is therefore a critical bug.

@jasonvarga can you take a look and merge if possible?

okaufmann commented 1 year ago

The overwrites are real, I just checked a project where I use the latest version of eloquent-driver and some submissions got overwritten… Fortunately, Emails were sent for the certain submissions, so the data isn't lost.

@enespolat24 would you mind adding some tests, which cover these two bugs, so they never happen again?

enespolat24 commented 1 year ago

I'm working on it

enespolat24 commented 1 year ago

@okaufmann i made some changes. I don't fully understand the overwrite situation. Do you have a chance to send an example case or video?

Thank you for your help !

enespolat24 commented 1 year ago

i also need help about tests

ryanmitchell commented 1 year ago

@enespolat24 I have the tests working... can you add me to your fork so I can push to it?

okaufmann commented 1 year ago

@enespolat24 Sorry for the delay.

I don't fully understand the overwrite situation. Do you have a chance to send an example case or video? In the current state, the date field will not be saved. Therefore, a form can only have one submission (as the filter is form_id and created_at, where created_at is always null).

@ryanmitchell Is there anything I could help? I have time now

ryanmitchell commented 1 year ago

@okaufmann i added a test here - seems to work ok for me: https://github.com/statamic/eloquent-driver/blob/cc3780c3a9654346206cb1de4ba96a9c181ca234/tests/Forms/FormSubmissionTest.php#L56

okaufmann commented 1 year ago

@ryanmitchell I would also freeze the time, so it creates two submissions width the same timestamp. So we can be sure it does not overwrite any submission.

ryanmitchell commented 1 year ago

@okaufmann that will fail due to the unique constraint in the database schema. Two submissions should never have the same microsecond time...

okaufmann commented 1 year ago

@ryanmitchell You're right. Then the tests cover all cases 💪

enespolat24 commented 1 year ago

@jasonvarga can you check if this is mergeable. This fixes a serious problem we face