Skip to content

Conversation

@mshannaq
Copy link
Contributor

@mshannaq mshannaq commented Aug 15, 2023

Redirect after login to entrance url
closes #722

@datamweb datamweb changed the title Redirect after login to entrance url #722 feat: redirect after login to entrance url Aug 15, 2023
@datamweb datamweb added enhancement New feature or request tests needed Pull requests that need tests labels Aug 15, 2023
@datamweb
Copy link
Collaborator

Thank you very much for your PR.

Please note that commit writing must be with the following description:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#committing

Please write unit test for PR:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

@kenjis
Copy link
Member

kenjis commented Aug 15, 2023

The develop branch has been updated and fixed the coding style check errors.
Please rebase to update your PR branch.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

@datamweb
Copy link
Collaborator

@mshannaq Please do not use git merge.
Use git rebase to update the branch.
Using git merge caused the github action to fail:
https://github.com/codeigniter4/shield/actions/runs/5891110836/job/15977524828?pr=789

@datamweb
Copy link
Collaborator

It seems you have 2 ways to fix error Detect Merge Commits.

  1. use git rebase -i
  2. use git cherry-pick

I try to use git rebase -i to fix the Github Action:

git branch beforeLogginUrl.bk beforeLogginUrl
git rebase -i origin/beforeLogginUrl~5

After your above command, your editor will open as follows.

Screenshot 2023-08-18 185910

Now modify the items in the editor as follows. And after that save the file and close the editor:

Screenshot 2023-08-18 191009

Now wait a bit. If you are asked for a password, enter your password. Your editor will open again as follows:

Screenshot 2023-08-18 191337

You should edit the file as follows and then save it. After that, close the editor:

Screenshot 2023-08-18 191802

If you have done everything correctly, you should see the following message in your terminal.

Screenshot 2023-08-18 191918

git push -f origin beforeLogginUrl

The above method is called squash. After executing the commands, your 5 commits should become 1 commit with message feat: redirect after login to entrance url.

To be honest, these topics are complicated for me too, I hope you can handle it.

@kenjis
Copy link
Member

kenjis commented Aug 18, 2023

@datamweb
How to git rebase is described here.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

$ git log --oneline 
8bf8c4f (HEAD -> beforeLogginUrl) feat: redirect after login to entrance url
379a201 Merge branch 'beforeLogginUrl' of https://github.com/mshannaq/ci4-shield into beforeLogginUrl
03f3a2c feat: redirect after login to entrance url
b099984 Merge branch 'codeigniter4:develop' into beforeLogginUrl
78cac54 (upstream/develop, origin/develop, origin/HEAD, develop) Merge pull request #788 from codeigniter4/datamweb-add-status-badge
89549c3 Merge pull request #782 from kenjis/update-RELEASE.md
2d0ec09 Merge pull request #787 from datamweb/fix-style-code
27b4323 Redirect after login to entrance url #722

Update your develop branch:

$ git fetch upstream
$ git switch develop
$ git merge upstream/develop
$ git push origin develop

Switch to the PR branch:

$ git switch beforeLogginUrl
Switched to branch 'beforeLogginUrl'

Do git rebase:

$ git rebase upstream/develop
Successfully rebased and updated refs/heads/beforeLogginUrl.
$ git log --oneline 
0ace028 (HEAD -> beforeLogginUrl) feat: redirect after login to entrance url
ac9173c feat: redirect after login to entrance url
97d92be Redirect after login to entrance url #722
78cac54 (upstream/develop, origin/develop, origin/HEAD, develop) Merge pull request #788 from codeigniter4/datamweb-add-status-badge

@kenjis
Copy link
Member

kenjis commented Aug 18, 2023

@mshannaq After git rebase, there are three commits. But it seems there is no reason to be three commits. The commit messages are the same, and they are all for redirect after login to entrance url.

If you can, please squash them into one commit.

{
$url = setting('Auth.redirects')['login'];
$session = session();
$url = $session->getTempdata('beforeLogginUrl') ?? setting('Auth.redirects')['login'];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$url = $session->getTempdata('beforeLogginUrl') ?? setting('Auth.redirects')['login'];
$url = $session->getTempdata('beforeLoginUrl') ?? setting('Auth.redirects')['login'];


if (! url_is('login')) {
$session = session();
$session->setTempdata('beforeLogginUrl', current_url(), 300);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$session->setTempdata('beforeLogginUrl', current_url(), 300);
$session->setTempdata('beforeLoginUrl', current_url(), 300);

Comment on lines +90 to +92
$result->assertRedirectTo('/login');
$this->assertNotEmpty($_SESSION['beforeLogginUrl']);
$this->assertSame(site_url('protected-route'), $_SESSION['beforeLogginUrl']);
Copy link
Member

Choose a reason for hiding this comment

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

All superglobals should be removed. So please use the Session class.

Suggested change
$result->assertRedirectTo('/login');
$this->assertNotEmpty($_SESSION['beforeLogginUrl']);
$this->assertSame(site_url('protected-route'), $_SESSION['beforeLogginUrl']);
$result->assertRedirectTo('/login');
$session = session();
$this->assertNotEmpty($session->get('beforeLogginUrl'));
$this->assertSame(site_url('protected-route'), $session->get('beforeLogginUrl'));

@mshannaq mshannaq closed this Aug 20, 2023
@mshannaq mshannaq deleted the beforeLogginUrl branch August 20, 2023 18:39
@mshannaq
Copy link
Contributor Author

I created a new branch and apply the changes into as it more easy and then I submit a PR793

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

Labels

enhancement New feature or request tests needed Pull requests that need tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants