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

after delete & redirect from Edit page, react-admin still does a refresh on edit page calling getOne on the deleted object #5541

Closed
bayareacoder opened this issue Nov 17, 2020 · 21 comments

Comments

@bayareacoder
Copy link

bayareacoder commented Nov 17, 2020

Edit page, undoable is set to false.
As soon as we press confirm on Delete, react-admin issues a delete and then a getOne on the deleted record.
This causes a rejected promise from the data provider (since the object is not found) which shows an 'element not found' popup in react-admin UI. We have a basePath and list view set for redirection and redirection happens, but it seems too late (after the page refresh).

Similarly with undoable set to true, we see a CRUD_GET_ONE action being pushed onto the optimisticCalls array which will issue a getOne after the server sends the response from delete.

There should be a redirect to list page PRIOR to the refresh as in useDeleteWithConfirmController.ts redirect is called before refresh (we do not use our own onSuccess handler). We would expect a getList to be called to refresh list view, not getOne.

const [deleteOne, { loading }] = useDelete(resource, null, null, {
        action: CRUD_DELETE,
        onSuccess: response => {
            setOpen(false);
            if (onSuccess === undefined) {
                notify('ra.notification.deleted', 'info', { smart_count: 1 });
                redirect(redirectTo, basePath);
                refresh();
            } else {
                onSuccess(response);
            }
        }, 

It seems the redirection comes too late. See screenshots:

  1. press the confirm button in Dialog. With breakpoint set on refresh, screen still shows the Edit page while the URL bar already shows the path for the list view.
  2. breakpoint in data provider's getOne, browser still shows Edit page.
  3. then react-admin shows list view (and it will pop-up the 'element not found' notification)

Is there a race condition so redirect does not complete prior to the refresh?
How can we fix this?

React-admin v.3.10.1

@bayareacoder bayareacoder changed the title after delete & redirect from Edit page, reaction-admin still does a refresh on edit page calling getOne on the deleted object after delete & redirect from Edit page, react-admin still does a refresh on edit page calling getOne on the deleted object Nov 17, 2020
@djhi
Copy link
Collaborator

djhi commented Nov 17, 2020

Thanks for reporting this. Please provide a sample application showing the issue by forking the following CodeSandbox (https://codesandbox.io/s/github/marmelab/react-admin/tree/master/examples/simple).

@bayareacoder
Copy link
Author

bayareacoder commented Nov 17, 2020

It will be difficult to provide that without copying a lot of our code since the issue doesn't show in the simple example given. The issue I believe comes in because we have a more complicated app so it may take longer for the redirected list page to paint. It's a custom list view.

Fundamentally I don't think you can assume that after a history.push the new location is loaded immediately. So issuing refresh(), right after redirect(), as you do in useDeleteWithConfirmController.ts may still cause the old page to be refreshed.

I'll be experimenting with a custom onSuccess handler to see if I can make the issue disappear.

@MicroJackson
Copy link

Maybe a dumb question but do you return the data of the specific item that you deleted?
Described here: https://marmelab.com/react-admin/DataProviders.html#delete

def delete(self, request, calculatie_id):
        calc_obj = Calculations.objects.get(id=calculatie_id)
        json_data = {
            "id": calc_obj.id,
            "customer_name": calc_obj.calculation_name,
            "offerte_nummer": calc_obj.calculation_number
        }

        calc_obj.delete()
        return HttpResponse(json.dumps(json_data, cls=DjangoJSONEncoder), content_type="application/json")

@bayareacoder
Copy link
Author

bayareacoder commented Nov 17, 2020

(yes we return the deleted record from the delete operation)

I have fixed the issue by doing this - I believe it confirms this is a race condition between redirection and refresh:

  • Supply an onSuccess handler to DeleteButton that overrides the default handler in useDeleteWithConfirmController.ts (see code in my original post)
  • Press the delete button and observe Redux actions.

Case 1 : with copy of existing code

<DeleteButton
     {...props}
     onSuccess={(response) => {
       notify('ra.notification.deleted', 'info', { smart_count: 1 });
       redirect(props.redirect, props.basePath);
       refresh();
     }}
/>

A CRUD_GET_ONE action is sent prior to CRUD_GET_LIST i.e. still on the Edit page on the deleted element. This is the issue reported.

Screen Shot 2020-11-17 at 9 51 59 AM

Case 2: with modified code (delayed refresh)

<DeleteButton
     {...props}
     onSuccess={(response) => {
       notify('ra.notification.deleted', 'info', { smart_count: 1 });
       redirect(props.redirect, props.basePath);
       setTimeout(refresh, 500);
     }}
/>

No CRUD_GET_ONE action is sent and the forced refresh now happens on the list page after redirection.

Screen Shot 2020-11-17 at 9 53 41 AM

(note: I modified the data provider to always return a valid response from getOne, i.e. returns {id : ''} when element is not found instead of a rejected promise to eliminate any different behavior from that)

The fix is ugly with the setTimeout but I believe it proves this is a genuine bug that may only show up in some cases (and since ra now uses react-router v5 I wouldn't be surprised there is different behavior due to that)

@fzaninotto
Copy link
Member

Interesting feedback, thanks a lot for the details.

Now we need to understand when the bug actually occurs, as we haven't been able to reproduce it so far. Could you try and reduce the conditions to reproduce the bug?

@bayareacoder
Copy link
Author

bayareacoder commented Nov 17, 2020

We actually have a deadline for a product introduction in the next few weeks that uses ra as an admin UI, so I'm a bit strapped for time right now.
There are different things in our app that could cause list view after redirect to take some time to complete rendering:

  • we have a custom list view that consists of multiple React components to create different 'pills'
  • custom styling
  • a data provider that is a 'master' provider that combines 2 different data providers, one for Firebase RT database, one for a GraphQL API, as explained here:
    Dumb dataProvider doesn't type check #5476 (comment)
  • graphQL API runs inside Docker locally during dev (web app with RA runs outside Docker)
  • runtime type checking on the data returned from the API inside the data provider to validate our API at the same time during dev. So especially on a getList it can take some extra time to return as each object in the array is checked.
  • running in debug mode in VSCode

All this can cause that it takes more time than typical (but definitely less than 500ms since the issue goes away with a delay of 500ms on the refresh) to paint the list view. So I would think mocking a list view in which you on purpose delay completing the render should have this issue show up.

Also I first tried to use the approach described here with the listener (https://stackoverflow.com/questions/45373742/detect-route-change-with-react-router) and call refresh() only inside the listener, but it didn't work: still the refresh came too early.

Fundamentally it needs some looking into react-router and history API to determine how to detect when it actually completes page load after history.push() and only issue refresh afterwards so your RA/REFRESH_VIEW action will be guaranteed to cause a re-render on the redirected page, I think.

@WiXSL
Copy link
Contributor

WiXSL commented Mar 19, 2021

#6063 ?

@AndriiPetrashchukEmpeek

Did you fix this bug, because I have the same bug on my project?

@WiXSL
Copy link
Contributor

WiXSL commented Mar 25, 2021

I'm just asking if this bug report is fixed with #6063, so this issue can be closed or is not related, since #5815 is fixed and is mentioned here.

@bayareacoder
Copy link
Author

bayareacoder commented Mar 25, 2021

I have no answer on this at this time. We are still using the delayed refresh workaround in a customized DeleteButton as explained in my earlier post and that works for us. #5815 seems different to me as we are not using a forced redirect.

@filip-jk
Copy link

filip-jk commented Apr 2, 2021

I'm still experiencing this issue: once the resource is deleted, React Admin redirects me to the List view and fetches the deleted resource with GET_ONE.

@dknight10
Copy link

Same issue here. An item gets deleted then a failure occurs when a get_one is called after the delete.

@WiXSL I don't believe the issue is related. I have upgraded to the most recent version and am running into the same problem.

@djhi
Copy link
Collaborator

djhi commented Apr 14, 2021

A codesandbox would help us to debug this.

@mindyk90
Copy link

mindyk90 commented May 3, 2021

Same issue here! It also occurs List view.

@acidjunk
Copy link

Not sure if it's related:
I thought I had this issue but then it seemed to originate from an react admin v2 dataProvider. Which was copied from the docs when I implemented it a while ago. The Delete didn't return data and whilst the delete went OK in the API -> the GUI showed a message about a missing ID. Attempts to add fix the notify worked with useNotify(): but then it didn't execute the call to the backend.

Added extra CASE for DELETE

    const convertHTTPResponse = (response, type, resource, params) => {
        const { headers, json } = response;
        switch (type) {
            case GET_LIST:
            case GET_MANY_REFERENCE:
                if (!headers.has("content-range")) {
                    throw new Error(
                        "The Content-Range header is missing in the HTTP Response. The simple REST data provider expects responses for lists of resources to contain this header with the total number of results to build the pagination. If you are using CORS, did you declare Content-Range in the Access-Control-Expose-Headers header?"
                    );
                }
                return {
                    data: json,
                    total: parseInt(headers.get("content-range").split("/").pop(), 10),
                };
            case CREATE:
                return { data: { ...params.data, id: json.id } };
            case DELETE_MANY: {
                return { data: json || [] };
            }
            case DELETE: {
                // Fix weird delete/redirect problem
                return { data: json || {} };
            }
            default:
                return { data: json };
        }
    };

Which solved it for me.

@Cornul11
Copy link
Contributor

I am having the exact same issue, after deleting the record, even if the delete occurred successfully, I still see a getOne request which yields an "Element not found" snackbar.

@TannicArturo98
Copy link

Inside useDeleteWithUndoController.tsx and useDeleteWithConfirmController.tsx files (path: node_modules/ra-core/src/controller/button/) it's called useRefresh hook inside onSuccess and onFailure functions and that's what it cause the getOne called.

I made a custom element without using useRefresh hook and it works fine:

const ServiceEditToolbar = (props: object) => {
    const notify = useNotify();
    const redirect = useRedirect();
    const [deleteOne, { loading }] = useDelete(props['resource'], props['record'].id, props['record'], {
        action: CRUD_DELETE,
        onSuccess: (response) => {
            notify('Deleted', 'info', undefined, true, 5000);
            redirect(props['basePath']);
        },
        onFailure: (error) => {
            notify(error.message, 'warning', undefined, false, 5000);
        }
    });

    return (
        <Toolbar {...props}>
            <Button onClick={deleteOne} disabled={loading} label="Elimina" />
        </Toolbar>
    )
};

@crates
Copy link

crates commented Dec 21, 2021

Huge thanks to @TannicArturo98 who seemed to have the most elegant solution of the ones listed here. This worked out great for us when we ran into the same React Admin bug as the others in this thread. Too bad we can't get an official fix or review from the Marmelab team since nobody knows precisely how to create a repro that manifests the issue.

@pacau999
Copy link

pacau999 commented Apr 1, 2022

I ran the same problem here, and compared my API response with json-api response(that does not trigger this bug) and discovered that if the server return the item object after delete call, react-admin will try to fetch the item.(??)

Solution: tweak the dataProvider or the API itself to return an empity object {}

@wc2184
Copy link

wc2184 commented May 4, 2022

Hearing what @pacau999 said, I did this:
Inside my Node.js api for the app.delete route, after deleting the data from my database I simply did
res.json( { } ), returning an empty object and then the issue was fixed. Before I had a res.send('Something') which resulted in this whole problem, so I just replaced that. I couldn't manage to fix it solely with clientside though.

@fzaninotto
Copy link
Member

This issue has been opened for a very long time without anyone providing a repro with CodeSandbox. We can't work on a bugfix if we can't replicate the bug. So I'm closing this issue.

If you have this bug, please open a new issue and attach a link to a CodeSandbox demonstrating it, following the issue template.

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

No branches or pull requests