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

Fix NoMethodError in order cycles index #2383

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Fix NoMethodError in order cycles index #2383

merged 1 commit into from
Jun 26, 2018

Conversation

frank-west-iii
Copy link
Collaborator

What? Why?

Closes #2271

When a user's session has timed out and they try to load new data on the
order cycles page by changing filters, the application throws a
NoMethodError because we are prepending the load data method before
checking the user's session.

We can fix this by removing the prepend on this action.

What should we test?

Loading and filtering the order cycles page. (Show 30 more days or Show 90 more days)

When a user's session has timed out and they try to load new data on the
order cycles page by changing filters, the application throws a
`NoMethodError` because we are prepending the load data method before
checking the user's session.

We can fix this by removing the prepend on this action.
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I would like to know why @oeoeaio added the prepend in 5176275. Since that commit is labelled WIP, it may have been just and experiment.

@mkllnk mkllnk requested a review from oeoeaio June 21, 2018 03:29
@oeoeaio
Copy link
Contributor

oeoeaio commented Jun 21, 2018

Hm, I can't remember exactly, and the specs are not that helpful, but I suspect it was something to do with altering the ransack query params in the load_data_for_index method, before the default collection setting logic from Spree::Admin::ResourceController was hit.

It looks like 2a5f598 rewrote most of this stuff anyway, and if the current version of the controller specs aren't broken by this change, then I think we just have to trust that everything is ok. We should still do rigorous testing around loading more order cycles using the button on the index though.

@mkllnk
Copy link
Member

mkllnk commented Jun 21, 2018

Thank you for the explanation @oeoeaio.

@sauloperez
Copy link
Contributor

Fingers crossed...

@mkllnk mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Jun 26, 2018
@mkllnk
Copy link
Member

mkllnk commented Jun 26, 2018

Staged on https://staging1.openfood.com.au/.

@RachL
Copy link
Contributor

RachL commented Jun 26, 2018

@mkllnk I've tried to login as spree@example on staging but it's not working. What kind of other admin account can I use to test this ? Thanks :)

@myriamboure
Copy link
Contributor

@RachL we use another user to test on Aus staging I got the credential I give them to you... I know it's still super messy we really need to have those common seeds data implemented please!!!

@RachL
Copy link
Contributor

RachL commented Jun 26, 2018

Thanks for the login info @myriamboure ! This is ready to go. However is there already somewhere an issue open in order to display a message in that case?
I'm afraid users are more likely to refresh their page when they see an error rather than an infinite spinner... but that's just my opinion.
Testing notes :
https://docs.google.com/document/d/12vNkQfzsBoKkXholzkCAPpHXzGQf4BC3ODAjPeFe86U/edit#

@RachL RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Jun 26, 2018
@mkllnk
Copy link
Member

mkllnk commented Jun 26, 2018

I just tested this myself, because the infinite spinner doesn't tell if the server encountered an error or not.

Previously, when you logged out in another window and then tried to load more order cycles, the server would throw an error. Now it recognises correctly, that the user is not logged in and redirects to the login. Since this happens in a JSON request, the Javascript doesn't know what to do with the result and just shows that infinite spinner. A user is most likely to refresh the page which will then prompt for login.

Pass. This is much better then before.

As Rachel said, we need a new issue for the infinite spinner though. In JSON requests, the redirect should be handled differently. It should be a valid JSON reply telling the browser to show the login modal (or redirect there).

@mkllnk mkllnk merged commit eb9064f into openfoodfoundation:master Jun 26, 2018
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

Successfully merging this pull request may close these issues.

NoMethodError in admin/order_cycles#index
6 participants