Fix session[:edit] breakage in picture controller.

[martinpovolny]

When picture controller is accessed the current content of
session[:edit] is lost. This is due to asymetry in
get_global_session_data vs set_global_session_data. One side always sets
the session[:edit] while the other does not always read it.

Furthemor the logic of {set/get}_global_session_data is not needed in
the picture controller.

Fixes: #2625
Replaces: #2747

ping @AparnaKarve, please, review, test, add a spec for this. Thank you!

[dclarizio]

Looks good to me . . . I wasn't aware that picture controller wasn't ACTUALLY involved in the edit session, so I like this fix . . . pending testing of course.

[martinpovolny]

Push forced to fix deprecation warning.

[miq-bot]

Checked commit martinpovolny@612b95d with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/controllers/picture_controller.rb

• ❗ - Line 3, Col 3 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

[AparnaKarve]

@martinpovolny First off, thank you for making the fix in the Picture Controller itself.
Glad, you did not venture making those changes in application_controller - what you suggested to me earlier. Whew!

I have no idea why you closed my PR (#2747), when you could have suggested me this solution in my PR (#2747) itself.

P.S. Going forward, please do not close my PRs without my knowledge. Thanks.

[AparnaKarve]

@martinpovolny Verified in the UI that the fix works.

I'm still thinking about the spec...
Maybe something as simple as checking if the prior session[:edit] value still persists after the picture GET request -

context 'skip_before_action :get_global_session_data / skip_after_action :set_global_session_data' do
  before do
    session[:edit] = "abc"
  end

  it 'retains the existing value of session[:edit] after the GET request' do
    get :show, :params => { :basename => "#{picture.compressed_id}.#{picture.extension}" }
    expect(session[:edit]).to eq("abc")
    expect(response.status).to eq(200)
  end
end

If you like it, add it to the PR.

[martinpovolny]

@AparnaKarve : My comment in your PR pointed out the 2 problematic lines. Yes, the solution was to avoid the 2 problematic lines. I did not say "Go into the ApplicationController and HACK IT there."

I fixed the issue by actually removing code from the execution path in the PictureController. So I actually disentangled part of the 🍝 .

What I am trying to explain to you (not only in this PR) is that fixing problematic places in code by adding lines that compensate around that in other places is bad. It's soooo bad, it's really a road to hell. This is our constant missunderstanding and source of friction although I honestly try to explain that from different angles.

At the time of writing the original comment I did not know that the best way to avoid the problematic lines is to avoid the whole before/after action.

To know that I needed to:

• open an editor, and
• browse around the code in both controllers,
• see that the PictureController does not need any @edit or current_user and then
• try it and
• see it worked.

I did that only after you wrote that you refuse to search for a proper fix and I should go and do it myself. At that point the simplest for me is to write git commit -a and hub pull-request and be done with it.

As for closing your PR. I apologize again. I wanted to avoid this long discussion by making a full stop. It was not a good idea and we probably need to tell all these things and make it straight and clear.

[martinpovolny]

Maybe something as simple as checking if the prior session[:edit] value still persists after the picture GET request -

yes, that makes perfect sense. Please, do.

[AparnaKarve]

@martinpovolny Thanks for the clarification above - #2756 (comment)

Based on your note, I think what happened yesterday was a communication mishap.

IMO, application_controller is like a restricted zone right now, because of all the GTL issues.
Your application_controller related comment #2747 (comment) did give an impression that you expected a fix in application_controller , which I just could not consider.
I figured, it was best if you did this yourself in application_controller, if that's where you were expecting a fix.

Hindsight is 20/20.
I think we could have avoided this if you had just told me in the PR, the exact change that you were expecting -- something that I would have happily implemented in the same PR.
(I like your skip_before_action/skip_after_action solution, btw)

One way to avoid this in the future would be to - adopt a model of brief and to-the-point messaging in the PRs (applies to me as well). Just a thought.

[AparnaKarve]

yes, that makes perfect sense. Please, do.

Created #2760 for the spec
Without this PR merged, I expect it to fail with this error -

expected: "abc"
     got: nil

[martinpovolny]

Ping @dclarizio, @himdel, @h-kataria, @mzazrivec : merge?

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 02bdbe84538f8a99d1e60295fb0e8c57ee90bf6d
Author: Harpreet Kataria <[email protected]>
Date:   Sat Nov 18 11:08:50 2017 -0500

    Merge pull request #2756 from martinpovolny/pictures_session_fix
    
    Fix session[:edit] breakage in picture controller.
    (cherry picked from commit a4f3759e2262b7620e9f048dddef063b1e956b27)