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

feat: Add ParseUser::logInAs method #486

Merged
merged 7 commits into from
Apr 29, 2023

Conversation

isaacaskew
Copy link
Contributor

@isaacaskew isaacaskew commented Apr 11, 2023

New Pull Request Checklist

Closes: #485

Issue Description

Later versions of Parse Server allow for the loginAs method. Given a userId and the master key, one can become logged in as the user associated to the userId provided. This PR adds the PHP Parse SDK method to call the Parse Server endpoint.

Approach

To implement this method, I've added a function similar to login but modified to only require the userId, as the master key is already assumed when using this method. Tests similar to the login tests have been added and modified to cover the cases of a success or a failure with no userId provided.

@parse-github-assistant
Copy link

parse-github-assistant bot commented Apr 11, 2023

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@isaacaskew isaacaskew changed the title Adding loginAs method with tests Feature: adding loginAs method with tests Apr 11, 2023
@isaacaskew isaacaskew changed the title Feature: adding loginAs method with tests feat: adding loginAs method with tests Apr 11, 2023
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: adding loginAs method with tests feat: Adding loginAs method with tests Apr 11, 2023
@mtrezza mtrezza requested a review from a team April 12, 2023 18:48
@mtrezza
Copy link
Member

mtrezza commented Apr 12, 2023

@dplewis Is this something you'd want to review?

mtrezza
mtrezza previously approved these changes Apr 12, 2023
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

I didn't know this was a feature. Probably should add it to the JS SDK

src/Parse/ParseUser.php Outdated Show resolved Hide resolved
@isaacaskew isaacaskew requested a review from dplewis April 21, 2023 23:00
Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@mtrezza mtrezza changed the title feat: Adding loginAs method with tests feat: Add loginAs method Apr 22, 2023
@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2023

@dplewis Before we merge this, do you have any idea how the test results are reported here? So we know that they pass before merging. This hasn't migrated to GitHub Actions yet and is still using Travis CI.

@dplewis
Copy link
Member

dplewis commented Apr 22, 2023

MIght have to reenable travis and pay the bill. I'm currenly working on migrating this to Github Action

dplewis#1

@isaacaskew
Copy link
Contributor Author

isaacaskew commented Apr 26, 2023

@mtrezza , @dplewis I forked this repo and tested it against these Parse Server Versions:

3.10.0 - [CircleCI run] [GitHub Changes] - Tests: 598, Assertions: 1334, Errors: 22, Failures: 4, Skipped: 1

5.4.3 - [CircleCI run] [GitHub Changes] - Tests: 598, Assertions: 1292, Errors: 29, Failures: 5, Skipped: 1

6.0.0 - [CircleCI run] [GitHub Changes] - Tests: 598, Assertions: 1272, Errors: 36, Failures: 5, Skipped: 1

There were 5 failures:

1) Parse\Test\ParseCloudTest::testGetJobsData
Failed asserting that 0 matches expected 3.

/Users/isaac/Development/parse-php-sdk/tests/Parse/ParseCloudTest.php:124

2) Parse\Test\ParseSessionFixationTest::testCookieIdChangedForAnonymous
Failed asserting that '' is not equal to "".

/Users/isaac/Development/parse-php-sdk/tests/Parse/ParseSessionFixationTest.php:51

3) Parse\Test\ParseSessionFixationTest::testCookieIdChangedForAnonymousToRegistered
Failed asserting that '' is not equal to "".

/Users/isaac/Development/parse-php-sdk/tests/Parse/ParseSessionFixationTest.php:65

4) Parse\Test\ParseSessionStorageTest::testIsUsingParseSession
Failed asserting that false is true.

/Users/isaac/Development/parse-php-sdk/tests/Parse/ParseSessionStorageTest.php:48

5) Parse\Test\ParseUserTest::testCountUsers
Failed asserting that 1 matches expected 3.

/Users/isaac/Development/parse-php-sdk/tests/Parse/ParseUserTest.php:587

Note that there were only 4 errors when run against Parse 3, and 5 errors in the other two. Let me know if there's anything about the server in particular I need to configure for these other ones to pass - otherwise I can try and help fix these once I understand a bit more of the context around a few of these functions.

In the meantime, Parse 6 running with my tests doesn't introduce any new test failures, so the 3 tests I added pass - we can see here that the pipeline run with my changes has the same number of errors as before, the 5 above. I chose Parse 6 as the server to run the tests against since Parse 6 is the version that introduced the new endpoint that I needed the client to talk to. I'm not in a rush to merge this PR so I'm game to help get the other tests passing and everything running clean against every version before we merge my feature in.

I can target the changes against a new major version of Parse PHP SDK as well - that might make sense since this method wouldn't work for any folks who upgrade their PHP client after this change is released but are using Parse Server versions < 6.

@mtrezza
Copy link
Member

mtrezza commented Apr 26, 2023

We've released the new SDK version 2.0.0. I've rebased this PR, let's see how tests are doing.

To your point, I think we should add a compatibility table to the README for Parse Server, to indicate which Parse Server versions this SDK is compatible with. For now, this is running against the alpha branch of Parse Server:

"parse-server": "github:parse-community/parse-server#alpha",

With all tests passing, we could assume that it's compatible with all relevant previous Parse Server versions - given that we don't support Parse Server 4 anymore, with Parse Server 5 currently being in LTS.

If you have any additions for loginAs in #491 please add them here. Once you are satisfied with this PR let us know and we can go ahead and merge it.

Changelog entry and version bump are not needed as part of this PR, auto-release will do that.

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (dfe2957) 98.47% compared to head (3e559b2) 98.48%.

❗ Current head 3e559b2 differs from pull request most recent head b6a6371. Consider uploading reports for the commit b6a6371 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #486   +/-   ##
=========================================
  Coverage     98.47%   98.48%           
- Complexity     1058     1060    +2     
=========================================
  Files            38       38           
  Lines          3213     3225   +12     
=========================================
+ Hits           3164     3176   +12     
  Misses           49       49           
Impacted Files Coverage Δ
src/Parse/ParseUser.php 98.47% <100.00%> (+0.07%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza
Copy link
Member

mtrezza commented Apr 28, 2023

I suppose we can merge this, it seems that #491 contains the same tests as this PR?

@dplewis
Copy link
Member

dplewis commented Apr 28, 2023

@mtrezza I think this is good to merge. You want to try auto release with this one?

@mtrezza
Copy link
Member

mtrezza commented Apr 29, 2023

Yes, let's give it a try! I'll just copy your fixes from #498 before that, so we should also see the docs auto-generated.

@mtrezza mtrezza changed the title feat: Add loginAs method feat: Add ParseUser::logInAs method Apr 29, 2023
@mtrezza mtrezza merged commit 5b4e102 into parse-community:master Apr 29, 2023
parseplatformorg pushed a commit that referenced this pull request Apr 29, 2023
# [2.1.0](2.0.0...2.1.0) (2023-04-29)

### Features

* Add `ParseUser::logInAs` method ([#486](#486)) ([5b4e102](5b4e102))
@parseplatformorg
Copy link

🎉 This change has been released in version 2.1.0

@parseplatformorg parseplatformorg added the state:released Released as stable version Edit label Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version Edit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to contribute
4 participants