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

Flashes are not getting reflashed on 409 #16

Closed
totaltrash opened this issue Feb 19, 2020 · 10 comments · Fixed by #24 or #26
Closed

Flashes are not getting reflashed on 409 #16

totaltrash opened this issue Feb 19, 2020 · 10 comments · Fixed by #24 or #26
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@totaltrash
Copy link

https://inertiajs.com/the-protocol#asset-versioning

Finally, in the event that flash session data exists when a 409 Conflict response occurs, the server will automatically reflash this data.

@tmartin8080 tmartin8080 added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Feb 19, 2020
@tmartin8080
Copy link
Collaborator

Thanks @totaltrash .

@bigx333
Copy link
Contributor

bigx333 commented Feb 19, 2020

fetch_flash does basically the same for redirects https://github.com/phoenixframework/phoenix/blob/master/lib/phoenix/controller.ex#L1296

Can use the same logic

@totaltrash
Copy link
Author

ugh, sorry, it's still early here. ignore my previous comment!

@totaltrash
Copy link
Author

I've had a nightmare with this. I can't write a failing test (I think it needs an integration test to replicate it), and I can't write an implementation... my elixir chops are not quite there.

Looking at how Phoenix handles flashes, as per your link @bigx333 , the flashes get deleted from the session before sending the response (unless a redirect). Se we'll need to store the flashes under a different session key, and merge them in upon the next request after a 409? Or something else?

@bigx333
Copy link
Contributor

bigx333 commented Feb 21, 2020

I've had a nightmare with this. I can't write a failing test (I think it needs an integration test to replicate it), and I can't write an implementation... my elixir chops are not quite there.

Looking at how Phoenix handles flashes, as per your link @bigx333 , the flashes get deleted from the session before sending the response (unless a redirect). Se we'll need to store the flashes under a different session key, and merge them in upon the next request after a 409? Or something else?

I didn't really have time to look into this but fetch_flash removes the session after it saves the flash on conn.private.phenix_flash, you should be able to just use put_session to persist the flash just like fetch_flash does and fetch_flash will automatically repopulate conn.private on the next request.

Something like this should work (untested)

|> check_flash()
|> send_resp(:conflict, "")
def check_flash(conn) do
  flash = conn.private.phoenix_flash
  flash_size = map_size(flash)

  if flash_size > 0 do
    put_session(conn, "phoenix_flash", flash)
  else
    conn
  end
end

Regarding the test, I'm not quite sure, normally on a phoenix test using get("/") would recycle the connection and set the session based on the cookies but here we don't have a router so this needs to be done manually.

@tmartin314 Any input on writing a test for this?

@tmartin8080
Copy link
Collaborator

Might be worth while to refactor the tests to use a dummy router and controller so we can test requests regularly. The controller test we have started out pretty simple and needs to cover many different scenarios now.

I'll start re-working the ConnCase.

@tmartin8080
Copy link
Collaborator

@totaltrash @bigx333 I was able to refactor the tests in 0.2.4. I'll take a stab at this issue as well. Thanks again for all your help.

tmartin8080 added a commit that referenced this issue Feb 22, 2020
Fix for #16 Forward flash on 409 conflict
@totaltrash
Copy link
Author

totaltrash commented Feb 24, 2020

G'day, I've pulled the latest changes including #24 into my test app. The flashes aren't getting reflashed upon a 409.

fetch_flash removes the session after it saves the flash on conn.private.phenix_flash, you should be able to just use put_session to persist the flash just like fetch_flash does and fetch_flash will automatically repopulate conn.private on the next request

I think the issue is that it clears the session before sending the response using register_before_send.

So the timing is:

  1. fetch_flash plug is called by the router, loading the flashes out of the session, and a callback is registered with register_before_send
  2. InertiaPhoenix.plug is called, an invalid inertia version is identified, and any existing flashes are written back to the session.
  3. The register_before_send is called, which deletes the session (as the status is not a redirect) including the reflashed flashes

@tmartin8080
Copy link
Collaborator

@totaltrash It looks like @bigx333 tackled this issue. It's merged into v0.2.6

@totaltrash
Copy link
Author

Yep, nailed it, nice @bigx333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
3 participants