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

Jw debugging cart remove add actions #730

Merged
merged 7 commits into from Nov 17, 2016

Conversation

jwiel86
Copy link
Contributor

@jwiel86 jwiel86 commented Nov 4, 2016

No description provided.

@@ -17,4 +17,4 @@
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
# TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
$('.cart').replaceWith("<%= escape_javascript(render( 'service_requests/right_navigation/cart', service_request: @service_request, sub_service_request: @sub_service_request, sub_service_requests: @sub_service_requests, line_items_count: @line_items_count, allow_delete: true )) %>")
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -571,7 +572,7 @@ def send_individual_service_provider_notification(service_request, sub_service_r
line_items = []
@service_request.sub_service_requests.each do |ssr|
if service_provider.identity.is_service_provider?(ssr)
line_items << SubServiceRequest.find(ssr).line_items
line_items << SubServiceRequest.find(ssr.id).line_items
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we finding the SubServiceRequest when we already have it from the .each iteration? Can't we do line_items << ssr.line_items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kyle-glick I was fixing a deprecation warning here, but yes! Good catch.

@@ -45,6 +42,8 @@ $(document).ready ->
$('#modal_place').modal('show')
$('#modal_place .yes-button').data('srid', srid)
$('#modal_place .yes-button').data('service-id', id)
$('#modal_place .yes-button').on 'click', (e) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a local handler? I noticed that the problem with the original code was that as you would remove/add more services, the handlers would add up and multiple clicks would be registered when you click the 'Yes' button. @jwiel86 Can you confirm?

If this does work, we should remove the two above lines setting data attributes on the yes button because they won't be needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kyle-glick When I was debugging this file, the variables set after $(document).on 'click', '.remove-service', -> were not being reset. So if you deleted one line_item, everything would work fine. However, if you deleted a second one. It would try to execute the code with the previous line item id and then it would also execute the code for the line item you just removed (I think that is what you were describing above- multiple clicks). Changing it to this selector on click format solved the problem for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure this onClick handler can only run once. Whole modal + .yes-button re-rendered on line 41.

@jleonardw9 jleonardw9 merged commit 439315c into master Nov 17, 2016
@jleonardw9 jleonardw9 deleted the jw_debugging_cart_remove_add_actions branch November 17, 2016 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants