[#73458] Missing feature specs on drag & drop

[dombesz]

Ticket

https://community.openproject.org/work_packages/73458

What are you trying to accomplish?

• Add missing specs for https://community.openproject.org/wp/73416

What approach did you choose and why?

• Check that project permissions are verified on the InboxController
• Use a real sign in workflow to reproduce the bug caused by the previous buggy prepend_before_action impementation.

Merge checklist

• Added/updated tests
• Added/updated documentation in Lookbook (patterns, previews, etc)
• Tested major browsers (Chrome, Firefox, Edge, ...)

[github-actions]

Caution

The provided work package version does not match the core version

Details:

• Work package URL: https://community.openproject.org/work_packages/73458
• Work package version: 17.3.0
• Core version: 17.4.0

Please make sure that:

• The work package version OR your pull request target branch is correct

[EinLama]

This PR should target release/17.3, not dev – or the version should be adjusted in the linked work package.

[EinLama]

I think something is not yet quite right here 🤔

The spec spec/features/oauth/authorization_code_flow_spec.rb is failing - and it is failing locally on my machine, too. I suspected it was related to unmocking the RequestStore - but I cannot make it green it by removing it.

Further investigation is necessary here.

[EinLama]

Interestingly, these two lines along with the password definition in the user variable can be removed and this test will still pass. So the logout hack is not necessary ☝️

This only works with the RequestStore unmocking in place 🤔 However, that one might be causing a failing spec (see other comment). If so, the unmocking should be moved here instead of residing in the authentication helper.

[EinLama]

Also, if the logout is not necessary, why does the unmocking have an effect? There might be other effects at play here.

[dombesz]

Interestingly, these two lines along with the password definition in the user variable can be removed and this test will still pass. So the logout hack is not necessary ☝️

Yes, that is correct. However if the real login is removed, the spec will still pass even without the fix applied, producing a false positive.

Having the real login flow will make the test fail if the fix is not applied, which is the correct behaviour.

[EinLama]

After some investigation, it seems like RequestStore is called with :anonymous_user in some situations. I think we might have to reset the mocks when logging out so that the original behavior is restored:

In authentication_helper:

  def login_as(user)
    # ...
    allow(RequestStore).to receive(:[]).and_call_original
    allow(RequestStore).to receive(:[]).with(:current_user).and_return(user)
  end

  # ...

  def logout
    # ...
    # Reset RequestStore stubs - restore original behavior for all keys:
    allow(RequestStore).to receive(:[]).and_call_original
  end

Sigh if only there was a web app without state 😅 Not sure if this is a good idea. But it makes both specs green. Maybe you'll find a better solution that is less frail.

[EinLama]

Thank you for fixing this 👏 What an intricate bug 😅