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

fix: updates the status for orders #2955

Merged
merged 2 commits into from
May 23, 2019

Conversation

shreyanshdwivedi
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi commented May 18, 2019

Fixes #2951

Short description of what this resolves:

Tickets that are unpaid are showing up as placed. Expected: Should show up as "Pending" and Expire after 3 days

Changes proposed in this pull request:

  • Updates the status of offline paid orders to Placed
  • Updates the status of online paid orders to Pending till payment is made.

Completed Order
Screenshot_2019-05-21 Completed Order -bffa6fb2-f09d-4f0a-ba82-35bca3121f75 Orders Open Event

Placed Order
Screenshot_2019-05-21 Placed Order -6e862478-d1a2-40e3-9ea6-8621528d0c13 Orders Open Event

Pending Order
Screenshot_2019-05-21 Placed Order -6c7479a6-220c-4baa-8346-fbabfe74a973 Orders Open Event

Expired Order
Screenshot_2019-05-21 Expired Order -be57dbe6-ff3d-4e09-b7e5-99680868db5b Orders Open Event

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)

@auto-label auto-label bot added the fix label May 18, 2019
@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented May 18, 2019

@uds5501 there will be a server side PR required just to make few changes -

  • update scheduler time constraint
  • ensure that placed order also gets similar completed_at updation, mailing and notification as it was getting earlier.

Edit: Will be pushing server side code in few minutes. Till then can you please verify if you are able to place orders with this changes.

uds5501
uds5501 previously approved these changes May 18, 2019
@fossasia fossasia deleted a comment May 18, 2019
@shreyanshdwivedi shreyanshdwivedi changed the title fix: updates the status for orders [WIP] fix: updates the status for orders May 18, 2019
@auto-label auto-label bot removed the fix label May 18, 2019
}
await order.save()
.then(order => {
if (order.status === 'placed') {
if (order.status === 'pending' || order.status === 'placed') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@shreyanshdwivedi If the order status is placed, ergo it was an order via bank cheques or something, why would we ask t fill payment details?

Copy link
Member Author

Choose a reason for hiding this comment

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

@CosmicCoder96 I agree. It was present earlier. I'll update it :)

@shreyanshdwivedi shreyanshdwivedi force-pushed the orderPage branch 2 times, most recently from 9f424df to 6887b74 Compare May 19, 2019 19:51
@fossasia fossasia deleted a comment May 19, 2019
@fossasia fossasia deleted a comment May 19, 2019
@shreyanshdwivedi shreyanshdwivedi changed the title [WIP] fix: updates the status for orders fix: updates the status for orders May 19, 2019
@auto-label auto-label bot added the fix label May 19, 2019
@fossasia fossasia deleted a comment May 19, 2019
@fossasia fossasia deleted a comment May 20, 2019
@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented May 20, 2019

@CosmicCoder96 @pradeepgangwar I've updated the color scheme like we decided

Expired -> red
Placed -> blue
Completed -> green 
Pending -> orange 

Please review
Edit: Screenshots are attached to PR description

@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 as corresponding server PR (fossasia/open-event-server#5926) is merged, can you please review this asap, otherwise the changes applied on server might break on local without these changes :)

Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Also use ES6 getters insted of this.get

I don't see the colour schema change logic for making initializing state orders as yellow?
@shreyanshdwivedi

<div class="ui inverted mini statistic horizontal">
<div class="value">
Success
{{#if (eq data.status 'completed')}}
Success
Copy link
Contributor

Choose a reason for hiding this comment

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

translation helpers.

{{#if (eq data.status 'completed')}}
Success
{{else}}
Placed
Copy link
Contributor

Choose a reason for hiding this comment

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

translation helpers.

<div class="ui inverted mini statistic horizontal">
<div class="value">
Placed
Pending
Copy link
Contributor

Choose a reason for hiding this comment

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

translation helpers.

@fossasia fossasia deleted a comment May 21, 2019
@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 I've added missing translation helpers.
Initializing is just an intermediate status. Once the form is submitted, the status is updated to either Pending, Placed or Completed. Therefore the color isn't required for this status
Regarding ES6 getters, do you want me to update all the getters in all the file related to this PR itself, as I haven't added any?

@fossasia fossasia deleted a comment May 21, 2019
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

@shreyanshdwivedi

I've added missing translation helpers.
Initializing is just an intermediate status. Once the form is submitted, the status is updated to either Pending, Placed or Completed.

What if I start an order, the timer starts, and then I close the tab. Then I login back and go to my tickets. The order will have a status?

Also if you didn't add any new getters yourself then leave them be, they are already being refactored by a different PR.

@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 you're absolutely right. I just checked, the status is getting updated to initializing. Pushing the changes
Also, I've added 3 new getters. Should I update it or will it be covered in the refactoring PR?

@abhinavk96
Copy link
Contributor

abhinavk96 commented May 21, 2019 via email

@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 I've pushed changes. BTW what I thought to be getters are not exactly getters. They all are something like - this.get('l10n').t(Pending Order -${order}). please review

@fossasia fossasia deleted a comment May 21, 2019
@fossasia fossasia deleted a comment May 21, 2019
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Fix merge conflicts.

@@ -3,7 +3,11 @@ import Route from '@ember/routing/route';
export default Route.extend({
titleToken(model) {
var order = model.order.get('identifier');
return this.get('l10n').t(`Completed Order -${order}`);
if (model.order.status === 'completed') {
return this.get('l10n').t(`Completed Order -${order}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

return this.l10n.t(...)

if (model.order.status === 'completed') {
return this.get('l10n').t(`Completed Order -${order}`);
} else if (model.order.status === 'placed') {
return this.get('l10n').t(`Placed Order -${order}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

return this.l10n.t(...)

fixes route issues and few condition checks

updates status of new orders

changes to orders.view template for completed and placed orders

Updates the color scheme of order status

Adds missing translation helpers

Update route and schema for initializing status

use ES6 getters
@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 I've fixed the merge conflicts. Please review

@shreyanshdwivedi
Copy link
Member Author

@pradeepgangwar @ritikamotwani please review

@abhinavk96 abhinavk96 merged commit e06fc4e into fossasia:development May 23, 2019
@shreyanshdwivedi shreyanshdwivedi deleted the orderPage branch May 25, 2019 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event Organizer Orders Page: Expired Orders Showing up as Placed Orders
4 participants