fix: Restructure and update role-invite flow #3091

uds5501 · 2019-06-08T10:26:04Z

Fixes #2771

Changes proposed in this pull request:

Fix transition syntax after the refactor

Checklist

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

uds5501 · 2019-06-08T10:32:25Z

Server PR : fossasia/open-event-server#6015

uds5501 · 2019-06-08T10:32:48Z

@kushthedude @shreyanshdwivedi @mrsaicharan1 Please review!

niranjan94

When doing async operations inside beforeModel ensure that beforeModel returns a promise that resolves once all async operations are completed.

uds5501 · 2019-06-10T05:26:53Z

@CosmicCoder96 @niranjan94 After this change, I think the element of network requests is resolved.
If I need to do something else too, please let me know.

uds5501 · 2019-06-10T05:45:33Z

@mrsaicharan1 @shreyanshdwivedi @kushthedude Review please

niranjan94

@uds5501 just putting an async before the function name isn't gonna solve this. The promise returned by beforeModel will still resolve before any of the two loaders within it complete their async task. You must await for those two tasks to complete.

niranjan94 · 2019-06-10T06:47:21Z

@shreyanshdwivedi @kushthedude please review the code before pressing approve, other-wise no point in having peer-reviews. :)

kushthedude · 2019-06-10T08:48:36Z

@uds5501 Can you make requested changes ASAP

uds5501 · 2019-06-10T09:01:35Z

@niranjan94 @CosmicCoder96 please have a look now

uds5501 · 2019-06-10T09:03:53Z

app/routes/public/role-invites.js

-          if (this.get('authManager.currentUser.email') === user.email) {
+        if (this.session.isAuthenticated) {
+          if (this.authManager.currentUser.email === user.email) {
            this.loader


@niranjan94 adding an await here threw a build error so removed it, will it be any problem for now?

@uds5501 you cannot use await without using async perhaps that's why you had gotten the error.

But the implementation is still wrong. When using async-await, don't use then-catch.

Please read up more on async-await.

shreyanshdwivedi · 2019-06-10T10:09:17Z

@niranjan94 actually the PR worked fine on my local. However didn't take a closer look on the code part. Will take care from next time

niranjan94 · 2019-06-10T10:35:27Z

app/routes/public/role-invites.js

-          if (this.get('authManager.currentUser.email') === user.email) {
+        if (this.session.isAuthenticated) {
+          if (this.authManager.currentUser.email === user.email) {
            this.loader


@uds5501 you cannot use await without using async perhaps that's why you had gotten the error.

niranjan94 · 2019-06-10T10:36:31Z

app/routes/public/role-invites.js

-          if (this.get('authManager.currentUser.email') === user.email) {
+        if (this.session.isAuthenticated) {
+          if (this.authManager.currentUser.email === user.email) {
            this.loader


But the implementation is still wrong. When using async-await, don't use then-catch.

Please read up more on async-await.

niranjan94 · 2019-06-10T10:37:21Z

actually the PR worked fine on my local. However didn't take a closer look on the code part. Will take care from next time

@shreyanshdwivedi Thanks ! Always look at the code itself before running it locally.

uds5501 · 2019-06-10T11:35:42Z

@niranjan94 in this commit I have implemented the second loader within another asynchronous function and user await withing that instead.
As for using then/catch, if I understood what you wanted correctly, I have used try/catch blocks instead for exception handling.

iamareebjamal · 2019-06-10T14:45:42Z

app/routes/public/role-invites.js

+      if (this.authManager.currentUser.email === user.email) {
+        await this.loader
+          .post('/role_invites/accept-invite', payload)
+          .then(invite => {


You're using await, then no need of then

@iamareebjamal Okay, I see the error. Fixing it

uds5501 · 2019-06-10T15:10:07Z

@iamareebjamal @CosmicCoder96 @niranjan94 Please review!!

niranjan94

@uds5501 there's a lot of code-duplication on this. I'm sure this can be much simpler and cleaner.

niranjan94 · 2019-06-10T17:47:19Z

app/routes/public/role-invites.js

+    try {
+      let user = await this.loader.post('/role_invites/user', payload);
+      if (this.session.isAuthenticated) {
+        this.routingModel(payload, user);


If routingModel is asynchronous, shouldn't you be awaiting it too ?

iamareebjamal · 2019-06-11T06:01:12Z

Not related to this PR, but use const always unless you need mutable variables

uds5501 · 2019-06-11T06:08:52Z

@niranjan94 I need a little help in reducing the repetition.

this.transitionTo('register', {
          queryParams: {
            event       : `${transition.resolvedModels.public.originalId}`,
            inviteToken : `${transition.to.queryParams.token}`,
            inviteEmail : `${user.email}`
          }
        });

is the repeating transition which can simply be performed in the beforeModel
IF this.transitionTo(getRoute, invite.event); doesn't happen. I tried remodelling the functions to

try {
      if (this.authManager.currentUser.email === user.email) {
        let invite = await this.loader.post('/role_invites/accept-invite', payload);
        let getRoute = (invite.role === 'organiser' || invite.role === 'coorganizer') ? 'events.view' : 'public';
        this.transitionTo(getRoute, invite.event);
        return true
      } else {
        this.set('session.skipRedirectOnInvalidation', true);
        this.session.invalidate();
        return false
      }
    }

and wanted to utilise this value to remove this redundant transition but I can't seem to be able to synchronise it.

niranjan94 · 2019-06-11T06:55:27Z

@uds5501 wouldn't this work ? This is an equivalent of your existing code but without code-repetition.

import Route from '@ember/routing/route';

export default Route.extend({
  async beforeModel(transition) {
    const { token } = transition.to.queryParams;
    const originalEventId = transition.resolvedModels.public.originalId;
    const payload = {
      data: { token }
    };

    const user = await this.loader.post('/role_invites/user', payload);

    if (this.session.isAuthenticated) {

      if (this.authManager.currentUser.email === user.email) {
        return this.transitionTo(
          ['organiser', 'coorganizer'].includes(invite.role) ? 'events.view' : 'public',
          await this.loader.post('/role_invites/accept-invite', payload)
        );
      }

      this.set('session.skipRedirectOnInvalidation', true);
      this.session.invalidate();
    }

    this.transitionTo('register', {
      queryParams: {
        event       : originalEventId,
        inviteToken : token,
        inviteEmail : user.email
      }
    });
  }
});

Also, lack of try catch is intentional here. We're letting ember handle the error and render the error page.

uds5501 · 2019-06-11T06:56:57Z

@uds5501 would this work ? This is an equivalent of your existing code but without code-repetition.

import Route from '@ember/routing/route';

export default Route.extend({
  async beforeModel(transition) {
    const { token } = transition.to.queryParams;
    const originalEventId = transition.resolvedModels.public.originalId;
    const payload = {
      data: { token }
    };

    const user = await this.loader.post('/role_invites/user', payload);

    if (this.session.isAuthenticated) {

      if (this.authManager.currentUser.email === user.email) {
        return this.transitionTo(
          ['organiser', 'coorganizer'].includes(invite.role) ? 'events.view' : 'public',
          await this.loader.post('/role_invites/accept-invite', payload)
        );
      }

      this.set('session.skipRedirectOnInvalidation', true);
      this.session.invalidate();
    }

    this.transitionTo('register', {
      queryParams: {
        event       : originalEventId,
        inviteToken : token,
        inviteEmail : user.email
      }
    });
  }
});

Also, lack of try catch is intentional here. We're letting ember handle the error and render the error page.

This should work! I will test it in my local and push the changes.

uds5501 · 2019-06-11T07:19:43Z

@niranjan94 it just took a minor change otherwise your code worked. Please review!

uds5501 · 2019-06-13T03:37:12Z

@niranjan94 @CosmicCoder96 please review

mariobehling

Moderators and all roles need to be able to see the dashboard, but depending on the role they don't have access to certain subpages. Please check how role models work on the UI of widely used systems like wordpress. We need to work on the details of roles, but this issue appears with all roles.

If I am an organizer I also receive an error page after an invite. Please take care of the organizer/co-organizer role first and ensure it is working.

The issue description of the linked issue simply says "does not work". This is not what we expect from an issue. Please write in future what behavior is expected - exactly -. So it can be solved accordingly.

No invited user should be directed to the public page. All invited event roles should be able to see the dashboard.

uds5501 · 2019-06-14T15:00:55Z

@mariobehling For now, I have made sure that organiser and co-organisers don't get a 400 error when accepting role invites. @niranjan94 @kushthedude @shreyanshdwivedi @mrsaicharan1 , Please review this

kushthedude · 2019-06-14T15:10:53Z

@uds5501 Any GIF or Demo of the following would be awesome

uds5501 · 2019-06-14T16:08:16Z

@kushthedude

kushthedude · 2019-06-14T16:10:52Z

@uds5501 Looks great, Just one more doubt what if the invite is of other roles, Is the user gets redirected to the event dashboard?

uds5501 · 2019-06-14T16:12:26Z

@uds5501 Looks great, Just one more doubt what if the invite is of other roles, Is the user gets redirected to the event dashboard?

@kushthedude
For now, let's focus on organisers and co-organisers. For the other roles, we will need to configure permissions individually in the server, I believe.

kushthedude · 2019-06-14T16:15:33Z

@uds5501 Looks great, Just one more doubt what if the invite is of other roles, Is the user gets redirected to the event dashboard?

@kushthedude
For now, let's focus on organisers and co-organisers. For the other roles, we will need to configure permissions individually in the server, I believe.

If that's the case, I think the PR is good to go then!

auto-label bot added the fix label Jun 8, 2019

kushthedude previously approved these changes Jun 8, 2019

View reviewed changes

shreyanshdwivedi previously approved these changes Jun 8, 2019

View reviewed changes

prateekj117 previously approved these changes Jun 8, 2019

View reviewed changes

niranjan94 suggested changes Jun 8, 2019

View reviewed changes

uds5501 dismissed stale reviews from prateekj117, shreyanshdwivedi, and kushthedude via a40b187 June 10, 2019 05:21

kushthedude previously approved these changes Jun 10, 2019

View reviewed changes

shreyanshdwivedi previously approved these changes Jun 10, 2019

View reviewed changes

niranjan94 suggested changes Jun 10, 2019

View reviewed changes

kushthedude added the Priority: Urgent label Jun 10, 2019

uds5501 dismissed stale reviews from shreyanshdwivedi and kushthedude via efdc61f June 10, 2019 09:00

uds5501 force-pushed the fix-2771 branch from b77dcff to efdc61f Compare June 10, 2019 09:00

uds5501 commented Jun 10, 2019

View reviewed changes

niranjan94 suggested changes Jun 10, 2019

View reviewed changes

iamareebjamal requested changes Jun 10, 2019

View reviewed changes

uds5501 force-pushed the fix-2771 branch from 6c5ed51 to 30029bd Compare June 10, 2019 15:09

niranjan94 suggested changes Jun 10, 2019

View reviewed changes

fossasia deleted a comment Jun 11, 2019

refactor the role acceptance flow

987ca60

uds5501 force-pushed the fix-2771 branch from f340bea to 987ca60 Compare June 11, 2019 07:19

fossasia deleted a comment Jun 11, 2019

niranjan94 previously approved these changes Jun 13, 2019

View reviewed changes

mariobehling requested changes Jun 14, 2019

View reviewed changes

fix role acceptance flow

baf2fea

uds5501 dismissed niranjan94’s stale review via baf2fea June 14, 2019 14:56

uds5501 changed the title ~~fix: Redirect user to public page if role is other than co-organizer~~ fix: Restructure and update role-invite flow Jun 14, 2019

kushthedude approved these changes Jun 14, 2019

View reviewed changes

abhinavk96 approved these changes Jun 14, 2019

View reviewed changes

abhinavk96 requested a review from niranjan94 June 14, 2019 16:38

niranjan94 merged commit 4d05048 into fossasia:development Jun 15, 2019

uds5501 deleted the fix-2771 branch June 18, 2019 15:52

fix: Restructure and update role-invite flow #3091

fix: Restructure and update role-invite flow #3091

Uh oh!

Conversation

uds5501 commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request:

Checklist

Uh oh!

uds5501 commented Jun 8, 2019

Uh oh!

uds5501 commented Jun 8, 2019

Uh oh!

niranjan94 left a comment

Choose a reason for hiding this comment

Uh oh!

uds5501 commented Jun 10, 2019

Uh oh!

uds5501 commented Jun 10, 2019

Uh oh!

niranjan94 left a comment

Choose a reason for hiding this comment

Uh oh!

niranjan94 commented Jun 10, 2019

Uh oh!

kushthedude commented Jun 10, 2019

Uh oh!

uds5501 commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyanshdwivedi commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niranjan94 commented Jun 10, 2019

Uh oh!

uds5501 commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uds5501 commented Jun 10, 2019

Uh oh!

niranjan94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamareebjamal commented Jun 11, 2019

Uh oh!

uds5501 commented Jun 11, 2019

Uh oh!

niranjan94 commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uds5501 commented Jun 11, 2019

Uh oh!

uds5501 commented Jun 11, 2019

Uh oh!

uds5501 commented Jun 13, 2019

Uh oh!

mariobehling left a comment

Choose a reason for hiding this comment

Uh oh!

uds5501 commented Jun 14, 2019

Uh oh!

kushthedude commented Jun 14, 2019

Uh oh!

uds5501 commented Jun 14, 2019

Uh oh!

kushthedude commented Jun 14, 2019

uds5501 commented Jun 8, 2019 •

edited

Loading

uds5501 commented Jun 10, 2019 •

edited

Loading

niranjan94 commented Jun 11, 2019 •

edited

Loading