Skip to content

enh: Moving Order_Expiry_Time into Admin/Settings #3228

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

enh: Moving Order_Expiry_Time into Admin/Settings #3228

wants to merge 14 commits into from

Conversation

@kushthedude
Copy link
Member

Fixes #3161

Short description of what this resolves:

  • Moved all the Order_Expiry_Time into Admin Dashboard

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)

@fossasia fossasia deleted a comment Jul 2, 2019
abhinavk96
abhinavk96 suggested changes Jul 3, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jul 3, 2019
@fossasia fossasia deleted a comment Jul 3, 2019
@fossasia fossasia deleted a comment Jul 3, 2019
@kushthedude kushthedude requested a review from abhinavk96 July 3, 2019 16:54
@fossasia fossasia deleted a comment Jul 3, 2019
@kushthedude kushthedude requested a review from niranjan94 July 3, 2019 16:59
abhinavk96
abhinavk96 previously approved these changes Jul 4, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jul 4, 2019
niranjan94
niranjan94 suggested changes Jul 4, 2019
View reviewed changes
niranjan94
niranjan94 suggested changes Jul 4, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jul 4, 2019
niranjan94
niranjan94 suggested changes Jul 5, 2019
View reviewed changes
@kushthedude
Copy link
Member Author

@niranjan94 @CosmicCoder96 Please Review !

@fossasia fossasia deleted a comment Jul 5, 2019
niranjan94
niranjan94 suggested changes Jul 5, 2019
View reviewed changes
app/components/forms/orders/order-form.js

getRemainingTime: computed('data', function() {
let orderExpiryTime = this.get('data.event.orderExpiryTime');
getRemainingTime: computed('settings', function() {
Copy link
Member

Choose a reason for hiding this comment

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

The computed property should be observing everything you are using. settings.orderExpiryTime, data.createdAt and data.identifier.

Please pay more attention to detail

app/components/forms/orders/order-form.js

getRemainingTime: computed('data', function() {
let orderExpiryTime = this.get('data.event.orderExpiryTime');
getRemainingTime: computed('settings', function() {
Copy link
Member

Choose a reason for hiding this comment

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

And what is this computer property returning ? I see no return statement at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

@niranjan94 , Same but it was implemented earlier this way, It's just used to calculate remaining time and this calls timer in it to calculate, But still no return it has

Copy link
Member Author

Choose a reason for hiding this comment

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

@niranjan94 I tried to return getRemaining but alas it was of no use due to the re-rendering of a rendered value, That's why I think this.set is used to overcome this challenge

Copy link
Member

Choose a reason for hiding this comment

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

This is very incorrect usage of a computed property. And a computed property should also never be reset by something else. Please refactor this correctly.

niranjan94
niranjan94 suggested changes Jul 6, 2019
View reviewed changes
app/components/forms/orders/order-form.js

getRemainingTime: computed('data', function() {
let orderExpiryTime = this.get('data.event.orderExpiryTime');
getRemainingTime: computed('settings', function() {
Copy link
Member

Choose a reason for hiding this comment

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

This is very incorrect usage of a computed property. And a computed property should also never be reset by something else. Please refactor this correctly.

@fossasia fossasia deleted a comment Jul 9, 2019
@shreyanshdwivedi
Copy link
Member

@kushthedude any update on this?

@kushthedude kushthedude deleted the expiry branch July 13, 2019 21:49
@kushthedude
Copy link
Member Author

I don't know what went wrong, All of my opened PR's are orphan, Will open a new one with a response to it

@kushthedude
Copy link
Member Author

Closed in favour of #3283

@kushthedude kushthedude mentioned this pull request Aug 3, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uds5501 uds5501 Awaiting requested review from uds5501

@shreyanshdwivedi shreyanshdwivedi Awaiting requested review from shreyanshdwivedi

@abhinavk96 abhinavk96 Awaiting requested review from abhinavk96

1 more reviewer

@niranjan94 niranjan94 niranjan94 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move registration time limit to admin settings and set standard to 15 minutes

4 participants

@kushthedude @shreyanshdwivedi @niranjan94 @abhinavk96