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

Disable AJAX for some Smart Answers #2682

Merged
merged 2 commits into from Aug 12, 2016

Conversation

Projects
None yet
3 participants
@selfthinker
Member

selfthinker commented Aug 9, 2016

Trello card

This disables the JS which loads questions via AJAX.
We would like to remove the AJAX altogether as it is not making things much quicker and it introduces maintability issues and bugs. To be reassured that this will not worsen the user experience, we only disable it on some Smart Answers.
The list of Smart Answers was provided by Dan Gilbert who chose some which are not visited too often or too seldom.

I also restructured the JS files to follow standard Rails conventions and to make sure smart-answers.js only contains JS relevant to the AJAX.

Expected changes

The pages energy-grants-calculator, calculate-employee-redundancy-pay and register-a-death will not use any AJAX calls in the background. The users will most probably not notice any difference. (We made this change to verify that assumption.) They will experience less bugs and very slightly longer delays when the page reloads.
As a developer you can check changes in Firefox by watching the console for those AJAX calls (which will be missing for the three Smart Answers mentioned above but be there for all other Smart Answers). In Chrome it can be checked in the console via the Network tab and filter by XHR.

@@ -0,0 +1,5 @@

This comment has been minimized.

@ikennaokpala

ikennaokpala Aug 11, 2016

Contributor

@selfthinker Very Minor: this line appears to be redundant.. can you take it away..

@ikennaokpala

ikennaokpala Aug 11, 2016

Contributor

@selfthinker Very Minor: this line appears to be redundant.. can you take it away..

energy-grants-calculator
calculate-employee-redundancy-pay
register-a-death
).exclude?(name)

This comment has been minimized.

@ikennaokpala

ikennaokpala Aug 11, 2016

Contributor

Building and maintaining this list to me is/will be brittle.. a solution could be for the Flow object to provide its ajax status.

This way we avoid having to building this list and avoid future increase/decrease in elements of the array here.

so just like @presenter provides the name of the Flow.

It would be better for a Flow ajax status to be set in the define method.

For example

def define
  content_id "yuewyeuw-ueiwueiw-uier76"
  name 'energy-grants-calculator'
  status :published
  satisfies_need "100982"
  ajax_enabled false  
....    

If this is not set the sensible default will be ajax_enabled true

And then in the presenter

 def ajax?
    @flow.ajax_enabled
 end
<script>
    var SmartAnswer = SmartAnswer || {};
    SmartAnswer.AJAX_ENABLED = <%= @presenter.ajax? %>;
</script>

@selfthinker also this will allow for tests to be written and sensible defaults.

We can also do this as a separate card from this.

@ikennaokpala

ikennaokpala Aug 11, 2016

Contributor

Building and maintaining this list to me is/will be brittle.. a solution could be for the Flow object to provide its ajax status.

This way we avoid having to building this list and avoid future increase/decrease in elements of the array here.

so just like @presenter provides the name of the Flow.

It would be better for a Flow ajax status to be set in the define method.

For example

def define
  content_id "yuewyeuw-ueiwueiw-uier76"
  name 'energy-grants-calculator'
  status :published
  satisfies_need "100982"
  ajax_enabled false  
....    

If this is not set the sensible default will be ajax_enabled true

And then in the presenter

 def ajax?
    @flow.ajax_enabled
 end
<script>
    var SmartAnswer = SmartAnswer || {};
    SmartAnswer.AJAX_ENABLED = <%= @presenter.ajax? %>;
</script>

@selfthinker also this will allow for tests to be written and sensible defaults.

We can also do this as a separate card from this.

This comment has been minimized.

@selfthinker

selfthinker Aug 11, 2016

Member

This is actually how I did it at first. But then I had a chat with @pmanrubia and he said it probably shouldn't be part of a flow.

@selfthinker

selfthinker Aug 11, 2016

Member

This is actually how I did it at first. But then I had a chat with @pmanrubia and he said it probably shouldn't be part of a flow.

This comment has been minimized.

@selfthinker

selfthinker Aug 11, 2016

Member

Also, please keep in mind that we don't plan to maintain that list. It is only temporary (for 2-3 weeks). That knowledge might change your approach.

@selfthinker

selfthinker Aug 11, 2016

Member

Also, please keep in mind that we don't plan to maintain that list. It is only temporary (for 2-3 weeks). That knowledge might change your approach.

@selfthinker

This comment has been minimized.

Show comment
Hide comment
@selfthinker

selfthinker Aug 11, 2016

Member

@ikennaokpala, everything you suggested except adding the status to the Flow object makes sense to me and I just made the corresponding changes, rebased and pushed.
Let's discuss the other suggested change with Pablo tomorrow.

Member

selfthinker commented Aug 11, 2016

@ikennaokpala, everything you suggested except adding the status to the Flow object makes sense to me and I just made the corresponding changes, rebased and pushed.
Let's discuss the other suggested change with Pablo tomorrow.

Restructure JS to use standard application.js
It is a Rails convention to use one `application.js` file
which then includes all other necessary files.
This does not change any behaviour, it only reorganises the JS.
Part of that is also to move JS irrelevant to the AJAX
out of smart-answers.js.
@@ -37,6 +37,10 @@ def current_state
@current_state ||= @flow.process(all_responses)
end
def name

This comment has been minimized.

@pmanrubia

pmanrubia Aug 12, 2016

Contributor

Minor: could you please add a test for this method?

@pmanrubia

pmanrubia Aug 12, 2016

Contributor

Minor: could you please add a test for this method?

@pmanrubia

This comment has been minimized.

Show comment
Hide comment
@pmanrubia

pmanrubia Aug 12, 2016

Contributor

@selfthinker: could you please update the card description with the steps we need to take to validate that the AJAX is disabled after it is deployed? This helps in our releases.
Something similar to:

Expected Changes

Visit these three URLs open the inspector and check…

Contributor

pmanrubia commented Aug 12, 2016

@selfthinker: could you please update the card description with the steps we need to take to validate that the AJAX is disabled after it is deployed? This helps in our releases.
Something similar to:

Expected Changes

Visit these three URLs open the inspector and check…

@pmanrubia

This comment has been minimized.

Show comment
Hide comment
@pmanrubia

pmanrubia Aug 12, 2016

Contributor

@selfthinker @ikennaokpala
Thank you for your work on this PR. I would prefer not to modify the workflow DSL to include UI specific concerns.

I would try to keep the workflow agnostic of the data presentation if possible. This is a bit more clear to me if I think of multiple devices with different presentation technologies.

Contributor

pmanrubia commented Aug 12, 2016

@selfthinker @ikennaokpala
Thank you for your work on this PR. I would prefer not to modify the workflow DSL to include UI specific concerns.

I would try to keep the workflow agnostic of the data presentation if possible. This is a bit more clear to me if I think of multiple devices with different presentation technologies.

@pmanrubia

This comment has been minimized.

Show comment
Hide comment
@pmanrubia

pmanrubia Aug 12, 2016

Contributor

@selfthinker
Well done creating the application.js 👍 . It is a good first step towards getting our JS sorted out.

Contributor

pmanrubia commented Aug 12, 2016

@selfthinker
Well done creating the application.js 👍 . It is a good first step towards getting our JS sorted out.

@ikennaokpala

This comment has been minimized.

Show comment
Hide comment
@ikennaokpala

ikennaokpala Aug 12, 2016

Contributor

@pmanrubia that is fine by me

Contributor

ikennaokpala commented Aug 12, 2016

@pmanrubia that is fine by me

Disable AJAX on some Smart Answers
This disables the JS which loads questions via AJAX.
We would like to remove the AJAX altogether
as it is not making things much quicker
and it introduces maintability issues and bugs.
To be reassured that this will not worsen the user experience,
we only disable it on some Smart Answers.
The list of Smart Answers was provided by Dan
who chose some which are not visited too often or too seldom.
@selfthinker

This comment has been minimized.

Show comment
Hide comment
@selfthinker

selfthinker Aug 12, 2016

Member

@pmanrubia, I have added the test with the help of @ikennaokpala and updated the Trello card description.

Member

selfthinker commented Aug 12, 2016

@pmanrubia, I have added the test with the help of @ikennaokpala and updated the Trello card description.

@selfthinker selfthinker merged commit caa3709 into master Aug 12, 2016

1 check passed

default "Build #5962 succeeded on Jenkins"
Details

@selfthinker selfthinker deleted the disable-ajax-for-some-smart-answers branch Aug 12, 2016

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