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

Create beforeConnect and beforeSubscribe Cloud Triggers #6649

Closed
wants to merge 45 commits into from
Closed

Create beforeConnect and beforeSubscribe Cloud Triggers #6649

wants to merge 45 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Apr 26, 2020

My first PR so feel free to make suggestions of course.

Creates two new cloud code triggers which affect LiveQuery: beforeConnect and beforeSubscribe.

beforeConnect allows validation prior to a LiveQuery opening connection. This is not class specific.

Parse.Cloud.beforeConnect(async request => {
      if (!request.user) {
          throw "Please login before you attempt to connect."
      }
  });

beforeSubscribe handles the .subscribe methods from LiveQuery. Can be used to validate users, or to mutate the request.query, enforcing fields, equalTo, or whatever required.

  Parse.Cloud.beforeSubscribe(Parse.User, async request => {
      if (!request.user) {
          throw "Please login before you attempt to connect."
      }
      let query = request.query; // the Parse.Query
      query.select("name","year")
      return query;
  });

I also created userForSessionToken on triggers.js, so that the sessionToken from LQ is only looked up if the trigger exists.

In relation to #6642.

@davimacedo
Copy link
Member

Thanks for the PR. Can you please check the tests out? They are not passing.

@dblythy
Copy link
Member Author

dblythy commented May 1, 2020

Will do!

@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #6649 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6649   +/-   ##
=======================================
  Coverage   93.59%   93.59%           
=======================================
  Files         169      169           
  Lines       12059    12059           
=======================================
  Hits        11287    11287           
  Misses        772      772           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eae38e9...eae38e9. Read the comment docs.

@dblythy
Copy link
Member Author

dblythy commented May 2, 2020

Sorry @davimacedo - not on my computer so had to edit through GitHub desktop which made it a bit difficult (and the commits a bit of a mess). I had to edit the LiveQuery spec so that it would await the result of the new beforeConnect / beforeSubscribe methods - is that okay?

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.

A few nits

src/triggers.js Outdated Show resolved Hide resolved
src/triggers.js Outdated Show resolved Hide resolved
src/triggers.js Outdated Show resolved Hide resolved
@@ -285,15 +285,15 @@ describe('ParseLiveQueryServer', function() {
.catch(done.fail);
});

it('can handle connect command', function() {
it('can handle connect command', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Adding await here is fine. Can you add additional tests for this feature? You might have to await in other places.

@dblythy
Copy link
Member Author

dblythy commented May 2, 2020

@dplewis thank you!

@dplewis
Copy link
Member

dplewis commented May 2, 2020

@dblythy Couldn't these trigger run in the background? (replace async with old fashion then / catch)

Edit:
beforeSubscribe doesn't need to return a query. You can mutate the query in the trigger and not enforce a return value.

beforeConnect in your example in the OP you assume its and async function returning

@dblythy
Copy link
Member Author

dblythy commented May 2, 2020

@dplewis I can work on replacing the async with then / catch. The only reason I used async is because my JavaScript is not great, especially with promises 😝. I will reopen another PR at a later date with tests as well. Thank you for your time and advice!

@dblythy dblythy closed this May 2, 2020
@dplewis
Copy link
Member

dplewis commented May 2, 2020

This is a great feature and a lot of progress was made. A lot of other data could be passed in too besides user and query. I look forward to your PR. If you don’t have time. Let me know and I can add this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants