Skip to content

fix: Restrict event save when information is insufficient #3305

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
Merged
merged 3 commits into from
Aug 13, 2019
Merged

fix: Restrict event save when information is insufficient #3305

merged 3 commits into from
Aug 13, 2019

Conversation

uds5501
Copy link
Contributor

@uds5501 uds5501 commented Jul 25, 2019

Fixes #2920

Short description of what this resolves:

The event save action will send appropriate error message if the following cases fail in the client level

  • Name of Event
  • Location
  • Dates
  • At least one Ticket exists

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 Jul 25, 2019
@uds5501 uds5501 force-pushed the event-check-errors branch from 64bce5f to 64052b4 Compare July 25, 2019 08:27
kushthedude
kushthedude suggested changes Jul 25, 2019
View reviewed changes
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

@uds5501 Why was this PR needed, Already checks for name, date, location are implemented and as far as tickets are concerned a server PR is already opened, And ticketing part was covered by @shreyanshdwivedi for FE ?

@uds5501
Copy link
Contributor Author

uds5501 commented Jul 25, 2019

@kushthedude
I thought a frontend application was required as well to handle this issue as per #2920 (comment)

@kushthedude
Copy link
Member

@kushthedude
I thought a frontend application was required as well to handle this issue as per #2920 (comment)

Arent the Semantic UI Validators enough for such ?

@kushthedude kushthedude requested a review from niranjan94 July 25, 2019 18:34
Anupam-dagar
Anupam-dagar previously approved these changes Jul 26, 2019
View reviewed changes
Copy link
Member

@Anupam-dagar Anupam-dagar left a comment

Choose a reason for hiding this comment

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

I guess if frontend validation is done then it's good to implement server side validation too.

abhinavk96
abhinavk96 suggested changes Jul 29, 2019
View reviewed changes
mrsaicharan1
mrsaicharan1 reviewed Jul 30, 2019
View reviewed changes
@uds5501 uds5501 force-pushed the event-check-errors branch from 5b616ed to 029cba5 Compare July 30, 2019 08:50
@fossasia fossasia deleted a comment Jul 30, 2019
@uds5501 uds5501 requested a review from mrsaicharan1 July 30, 2019 09:03
shreyanshdwivedi
shreyanshdwivedi previously approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi left a comment

Choose a reason for hiding this comment

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

LGTM for FE checks 👍

kushthedude
kushthedude previously approved these changes Jul 30, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jul 30, 2019
abhinavk96
abhinavk96 suggested changes Jul 31, 2019
View reviewed changes
app/mixins/event-wizard.js Outdated
}
}
}
const numberOfTickets = data.tickets.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

This code will completely crash if for some reason data.tickets is not defined.
Every where else, first it is checked if data.property exists. Data.undefined.length will throw an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CosmicCoder96 Understood, changing accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CosmicCoder96 Updated the assignment, please check now

@uds5501 uds5501 dismissed stale reviews from kushthedude and shreyanshdwivedi via f1b31cc July 31, 2019 04:22
@fossasia fossasia deleted a comment Jul 31, 2019
@uds5501 uds5501 requested a review from pradeepgangwar August 2, 2019 04:57
@uds5501
Copy link
Contributor Author

uds5501 commented Aug 2, 2019

@mrsaicharan1 @shreyanshdwivedi please take a look

mrsaicharan1
mrsaicharan1 previously approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

LGTM

@fossasia fossasia deleted a comment Aug 2, 2019
abhinavk96
abhinavk96 suggested changes Aug 2, 2019
View reviewed 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.

Squash and properly name commits.

@uds5501 uds5501 force-pushed the event-check-errors branch from f430c32 to 5733b32 Compare August 3, 2019 04:47
@uds5501 uds5501 force-pushed the event-check-errors branch from 5733b32 to ecf413f Compare August 3, 2019 04:48
@uds5501
Copy link
Contributor Author

uds5501 commented Aug 3, 2019 •
edited
Loading

@CosmicCoder96 Squashed the commits, please take a look

@fossasia fossasia deleted a comment Aug 3, 2019
shreyanshdwivedi
shreyanshdwivedi approved these changes Aug 3, 2019
View reviewed changes
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@abhinavk96 abhinavk96 merged commit 2a9e44f into fossasia:development Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niranjan94 niranjan94 Awaiting requested review from niranjan94

@pradeepgangwar pradeepgangwar Awaiting requested review from pradeepgangwar

5 more reviewers

@abhinavk96 abhinavk96 abhinavk96 approved these changes

@mrsaicharan1 mrsaicharan1 mrsaicharan1 approved these changes

@shreyanshdwivedi shreyanshdwivedi shreyanshdwivedi approved these changes

@kushthedude kushthedude kushthedude approved these changes

@Anupam-dagar Anupam-dagar Anupam-dagar left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Only allow "Publish Event" If the Event has Relevant Info

6 participants

@uds5501 @kushthedude @abhinavk96 @Anupam-dagar @mrsaicharan1 @shreyanshdwivedi