Skip to content

Link is lost when user logs in#6300

Merged
tramuntanal merged 10 commits intodecidim:developfrom
CodiTramuntana:keep_previous_link_when_user_logs_in
Aug 5, 2020
Merged

Link is lost when user logs in#6300
tramuntanal merged 10 commits intodecidim:developfrom
CodiTramuntana:keep_previous_link_when_user_logs_in

Conversation

@gemmatm
Copy link
Copy Markdown
Contributor

@gemmatm gemmatm commented Jul 14, 2020 •
edited
Loading

🎩 What? Why?

When a member recives a link, and does not have the session opened, the link takes him directly to the login screen. When the user is already logged, the user loses the link to which is supposed to go and redirects to the homepage, instead of going to the initial link.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • Add tests

📷 Screenshots (optional)

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Jul 15, 2020

Beware @gemmatm there are a lot of commits that do not correspond to the PR itself

@victorol1 victorol1 deleted the keep_previous_link_when_user_logs_in branch July 29, 2020 14:55
@victorol1 victorol1 force-pushed the keep_previous_link_when_user_logs_in branch from e6833cd to 0a6ae20 Compare July 30, 2020 09:04
@victorol1 victorol1 force-pushed the keep_previous_link_when_user_logs_in branch from 2b40b82 to ef8e34a Compare July 30, 2020 12:47
@victorol1 victorol1 force-pushed the keep_previous_link_when_user_logs_in branch 2 times, most recently from 857a3a4 to ba88c6e Compare July 31, 2020 11:56
@victorol1 victorol1 force-pushed the keep_previous_link_when_user_logs_in branch from ba88c6e to 4c80a4b Compare August 3, 2020 06:20
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Aug 4, 2020

Shouldn't this be implemented with the store_location_for method provided by Devise, as used already by Decidim:

decidim/decidim-core/app/controllers/decidim/application_controller.rb

Lines 55 to 60 in c964194

def store_current_location
return if skip_store_location?
value = redirect_url || request.url
store_location_for(:user, value)
end

You should be able to just call store_current_location before the redirection.

It's already called by Decidim but I guess it's not working in this case because the redirection happens before the location is stored.

@victorol1
Copy link
Copy Markdown
Contributor

victorol1 commented Aug 5, 2020

Shouldn't this be implemented with the store_location_for method provided by Devise, as used already by Decidim:

decidim/decidim-core/app/controllers/decidim/application_controller.rb

Lines 55 to 60 in c964194

def store_current_location
return if skip_store_location?
value = redirect_url || request.url
store_location_for(:user, value)
end

You should be able to just call store_current_location before the redirection.

It's already called by Decidim but I guess it's not working in this case because the redirection happens before the location is stored.

You are right, this makes it much easier! Thank you very much!

ahukkanen
ahukkanen reviewed Aug 5, 2020
View reviewed changes
decidim-core/app/controllers/concerns/decidim/force_authentication.rb
# Next stop: Let's check whether auth is ok
unless user_signed_in?
flash[:warning] = t("actions.login_before_access", scope: "decidim.core")
store_location_for(:user, request.path)
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen Aug 5, 2020

Choose a reason for hiding this comment

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

I think you could just call store_current_location here, right?

Copy link
Copy Markdown
Contributor

@victorol1 victorol1 Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It gives errors on some controllers while running tests if I do so

@victorol1 victorol1 force-pushed the keep_previous_link_when_user_logs_in branch from 3bbd983 to b97c765 Compare August 5, 2020 07:13
@tramuntanal tramuntanal marked this pull request as ready for review August 5, 2020 07:52
@tramuntanal tramuntanal merged commit dfd07c2 into decidim:develop Aug 5, 2020
Quentinchampenois pushed a commit to Quentinchampenois/decidim that referenced this pull request Aug 14, 2020
@gemmatm @Quentinchampenois
Link is lost when user logs in (decidim#6300)
1e923af 
* Change redirection after login

* Do not redirect if first login

* Refactor and remove before_action call

* Add test

* Refactor all code into devise method store_location_for

* Replace store_location_for with store_current_location

* Revert latest changes

Co-authored-by: Víctor <victor.ol@coditramuntana.com>
@leio10 leio10 mentioned this pull request Jun 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen left review comments

+2 more reviewers

@victorol1 victorol1 victorol1 left review comments

@tramuntanal tramuntanal tramuntanal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Link is lost when user logs in

4 participants

@gemmatm @tramuntanal @ahukkanen @victorol1