feat: implement resend email confirmation button #3251

uds5501 · 2019-07-10T06:20:13Z

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

Short description of what this resolves:

The PR implements the utlizes rate limited end point on the server side to send order confirmation for completed and placed tickets.

Changes proposed in this pull request:

Use payloads and display appropriate error messages

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)

niranjan94

@uds5501 I have told this before. Do not create frontend PRs without the server code being merged in. When something depends on the server APIs, it is not possible to test the frontend code until server is merged in.

niranjan94 · 2019-07-10T12:46:21Z

app/controllers/events/view/tickets/orders/list.js

+          }
+        };
+        let resend = await this.loader.post('orders/resend-email', payload);
+        if (resend.status) {


Comment regarding this added on the server PR. fossasia/open-event-server#6163 (comment)

niranjan94 · 2019-07-10T12:47:35Z

Re-open after server PR is merged

app/controllers/events/view/tickets/orders/list.js

niranjan94 · 2019-07-13T15:01:41Z

app/controllers/events/view/tickets/orders/list.js

+          }
+        };
+        let resendEmail = await this.loader.post('orders/resend-email', payload);
+        if (resendEmail.status) {


@uds5501 update this to the correct error handling. If this.loader.post succeeds it should be considered a success. Any error should be in the catch block

@niranjan94 Did it!

niranjan94 · 2019-07-13T16:24:21Z

app/controllers/events/view/tickets/orders/list.js

+        await this.loader.post('orders/resend-email', payload);
+        this.notify.success(this.l10n.t('Email confirmation has been sent to attendees successfully'));
+      } catch (error) {
+        this.notify.error(this.l10n.t('An unexpected error occurred.'));


What was the error ? Could we display something better ? Like for example, if it was a ratelimitting error, show that it was due to it. Else show whatever the actual error sent by the backend is.

@niranjan94 I have changed error handling, is it any better?

niranjan94 · 2019-07-13T17:40:30Z

app/controllers/events/view/tickets/orders/list.js

+        this.notify.success(this.l10n.t('Email confirmation has been sent to attendees successfully'));
+      } catch (error) {
+        console.log(error);
+        const errorMessage = error.message || error.errors[0].detail;


What about the case where the rate limiting kicks in ? What is the error message sent by the server ? Does it come in here correctly ?

@niranjan94 When rate limit kicks in, it's handled by error.message. If it's other trivial error like sending confirmation for cancelled orders, it's handled by error.errors[0].detail. Otherwise the else condition.

The rate limiting error sent is : TOO MANY REQUESTS.

@uds5501 for rate limiting the server would be sending a 429 if I'm right. When that comes, please show a proper and a more human-friendly error message

Also, with translations

niranjan94 · 2019-07-13T17:40:59Z

app/controllers/events/view/tickets/orders/list.js

+        this.notify.success(this.l10n.t('Email confirmation has been sent to attendees successfully'));
+      } catch (error) {
+        console.log(error);
+        const errorMessage = error.message || error.errors[0].detail;


Give first priority to error.errors[0].detail. Also, what if server is sending multiple errors. You should not hard code to display only the first.

@niranjan94 Understood

uds5501 · 2019-07-16T11:16:43Z

app/controllers/events/view/tickets/orders/list.js

+        await this.loader.post('orders/resend-email', payload);
+        this.notify.success(this.l10n.t('Email confirmation has been sent to attendees successfully'));
+      } catch (error) {
+        if (error.message === 'TOO MANY REQUESTS') {


@niranjan94 @shreyanshdwivedi @CosmicCoder96 Will this workaround be okay?

@uds5501 this is very wrong. You must use the status code.

Modify the loader service to ensure the original response is sent as the error along with the message that is being currently sent there

https://github.com/fossasia/open-event-frontend/blob/development/app/services/loader.js#L100

@niranjan94 Understood.

niranjan94 · 2019-07-17T18:53:58Z

app/controllers/events/view/tickets/orders/list.js

+        await this.loader.post('orders/resend-email', payload);
+        this.notify.success(this.l10n.t('Email confirmation has been sent to attendees successfully'));
+      } catch (error) {
+        if (error.message === 'TOO MANY REQUESTS') {


@uds5501 this is very wrong. You must use the status code.

Modify the loader service to ensure the original response is sent as the error along with the message that is being currently sent there

https://github.com/fossasia/open-event-frontend/blob/development/app/services/loader.js#L100

niranjan94

@uds5501 try not to find shortcuts. Spend some more time on fixes. See how you can make it such that its re-usable

niranjan94 · 2019-07-20T06:07:53Z

app/services/loader.js

      if (parsedResponse) {
        throw parsedResponse;
      }
+      if (response.status === 429) {


No. Make this a generic implementation such that status code and the original response is always passed in.

if (!response.ok) { const defaultMessage = httpStatus[response.status]; const errorResponse = pick(response, ['status', 'ok', 'statusText', 'headers', 'url']); errorResponse.statusText = defaultMessage; errorResponse.response = parsedResponse; errorResponse.errorMessage = isString(parsedResponse) ? parsedResponse : getErrorMessage( response.statusText, defaultMessage ? `${response.status} - ${defaultMessage}` : `Could not make ${fetchOptions.type} request to ${fetchOptions.url}` ); throw errorResponse; }

uds5501 · 2019-07-24T07:38:15Z

@mrsaicharan1 @shreyanshdwivedi @CosmicCoder96 Please review this

kushthedude · 2019-07-24T07:55:56Z

app/controllers/events/view/tickets/orders/list.js

+        this.notify.success(this.l10n.t('Email confirmation has been sent to attendees successfully'));
+      } catch (error) {
+        if (error.status === 429) {
+          this.notify.error(this.l10n.t('Only 5 resend actions are allowed in a minute'));


What if the error is something other than the two you specified.
Comparing error returned by server with the codes is not a optimisable solution its more like a workaround. @uds5501 is this the only working way for this problem ?

@kushthedude There's no context of optimality here. @uds5501 Just add an else statement for other errors & this is good to go :D

app/controllers/events/view/tickets/orders/list.js

tests/integration/components/ui-table/cell/events/view/tickets/orders/cell-order-test.js

app/controllers/events/view/tickets/orders/list.js

abhinavk96 · 2019-08-02T18:51:08Z

app/controllers/events/view/tickets/orders/list.js

+      let payload = {};
+      try {
+        payload = {
+          'data': {


order_identifier = request.json['data']['order'] order = safe_query(db, Order, 'identifier', order_identifier, 'identifier') if (has_access('is_coorganizer', event_id=order.event_id)): if order.status == 'completed' or order.status == 'placed': # fetch tickets attachment order_identifier = order.identifier key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order_identifier) ticket_path = 'generated/tickets/{}/{}/'.format(key, generate_hash(key)) + order_identifier + '.pdf' key = UPLOAD_PATHS['pdf']['order'].format(identifier=order_identifier) invoice_path = 'generated/invoices/{}/{}/'.format(key, generate_hash(key)) + order_identifier + '.pdf' # send email. send_email_to_attendees(order=order, purchaser_id=current_user.id, attachments=[ticket_path, invoice_path]) return jsonify(status=True, message="Verification emails for order : {} has been sent succesfully". format(order_identifier)) else: return UnprocessableEntityError({'source': 'data/order'}, "Only placed and completed orders have confirmation").respond() else: return ForbiddenError({'source': ''}, "Co-Organizer Access Required").respond()

This is the code for this end point on the server,
why is it the user field required?

User is being used for rate limiting for users

abhinavk96

That is not how commits should be, squash them use semantic naming.

uds5501 · 2019-08-13T14:35:09Z

@CosmicCoder96 @mrsaicharan1 @shreyanshdwivedi @kushthedude Please review this.

auto-label bot added the feature label Jul 10, 2019

uds5501 added the blocked-by-server label Jul 10, 2019

niranjan94 suggested changes Jul 10, 2019

View reviewed changes

niranjan94 closed this Jul 10, 2019

uds5501 reopened this Jul 13, 2019

uds5501 removed the blocked-by-server label Jul 13, 2019

uds5501 force-pushed the resend-route-action branch from 25dc235 to 56d14d1 Compare July 13, 2019 06:12

uds5501 requested review from kushthedude, mrsaicharan1, niranjan94 and shreyanshdwivedi July 13, 2019 06:13

kushthedude suggested changes Jul 13, 2019

View reviewed changes

app/controllers/events/view/tickets/orders/list.js Outdated Show resolved Hide resolved

uds5501 requested a review from kushthedude July 13, 2019 09:05

kushthedude suggested changes Jul 13, 2019

View reviewed changes

app/controllers/events/view/tickets/orders/list.js Outdated Show resolved Hide resolved

kushthedude previously approved these changes Jul 13, 2019

View reviewed changes

niranjan94 suggested changes Jul 13, 2019

View reviewed changes

uds5501 dismissed kushthedude’s stale review via dfa01da July 13, 2019 15:54

uds5501 force-pushed the resend-route-action branch from f3cd499 to dfa01da Compare July 13, 2019 15:54

uds5501 requested review from kushthedude and niranjan94 July 13, 2019 15:55

niranjan94 suggested changes Jul 13, 2019

View reviewed changes

uds5501 force-pushed the resend-route-action branch from dfa01da to f8c6ac4 Compare July 13, 2019 17:28

niranjan94 suggested changes Jul 13, 2019

View reviewed changes

uds5501 force-pushed the resend-route-action branch from f8c6ac4 to 427011a Compare July 13, 2019 17:59

uds5501 requested review from abhinavk96 and niranjan94 July 14, 2019 03:20

uds5501 force-pushed the resend-route-action branch from 427011a to af928c0 Compare July 16, 2019 06:45

uds5501 commented Jul 16, 2019

View reviewed changes

niranjan94 suggested changes Jul 17, 2019

View reviewed changes

uds5501 force-pushed the resend-route-action branch from af928c0 to d529408 Compare July 17, 2019 23:53

uds5501 requested a review from niranjan94 July 17, 2019 23:56

niranjan94 suggested changes Jul 20, 2019

View reviewed changes

uds5501 force-pushed the resend-route-action branch from 7697d01 to 15a1563 Compare July 20, 2019 17:13

uds5501 requested a review from niranjan94 July 20, 2019 17:19

uds5501 force-pushed the resend-route-action branch from 15a1563 to 4e215bb Compare July 21, 2019 06:28

kushthedude reviewed Jul 24, 2019

View reviewed changes

uds5501 force-pushed the resend-route-action branch from 4e215bb to b7463d5 Compare July 24, 2019 12:20

uds5501 requested a review from kushthedude July 25, 2019 10:02

mrsaicharan1 previously approved these changes Aug 2, 2019

View reviewed changes

abhinavk96 suggested changes Aug 2, 2019

View reviewed changes

uds5501 dismissed mrsaicharan1’s stale review via b2157c5 August 3, 2019 05:05

uds5501 force-pushed the resend-route-action branch 2 times, most recently from b2157c5 to 26436bf Compare August 3, 2019 05:06

feat: add resend confirmation workflow

b1d83cd

uds5501 force-pushed the resend-route-action branch from 26436bf to b1d83cd Compare August 3, 2019 06:10

uds5501 requested review from abhinavk96 and mrsaicharan1 August 3, 2019 06:16

kushthedude approved these changes Aug 13, 2019

View reviewed changes

Merge branch 'development' into resend-route-action

4b2bf60

abhinavk96 approved these changes Aug 13, 2019

View reviewed changes

abhinavk96 merged commit d6114bc into fossasia:development Aug 13, 2019

Uh oh!

feat: implement resend email confirmation button #3251

feat: implement resend email confirmation button #3251

Uh oh!

Conversation

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

Uh oh!

Short description of what this resolves:

Changes proposed in this pull request:

Checklist

Uh oh!

niranjan94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niranjan94 commented Jul 10, 2019

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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!

niranjan94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uds5501 commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavk96 left a comment

Choose a reason for hiding this comment

Uh oh!

uds5501 commented Aug 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

uds5501 commented Jul 10, 2019 •

edited

Loading

uds5501 Jul 13, 2019 •

edited

Loading