Skip to content

Conversation

@shreyanshdwivedi
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi commented May 15, 2019

Fixes #2482 , Related to #2920

Short description of what this resolves:

The option "use ticketing system" is a nice feature to add events of external websites. The options has been abused though to add spam events.

Changes proposed in this pull request:

  • Comments option use ticketing system

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)

</div>
<div class="field">
<label for="location">{{t 'Location'}}</label>
<label class="required" for="location">{{t 'Location'}}</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is to show a message if the location, date or ticketing is missing and the user tries to publish the event. Introducing the "required" label is just misleading. The event can be stored as a draft if these informations are missing. You've to add the correct messages not make them required.

Copy link
Member Author

@shreyanshdwivedi shreyanshdwivedi May 15, 2019

Choose a reason for hiding this comment

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

@ritikamotwani I'm currently working on it. I'll remove [WIP] label as soon as it gets completed 😅
I've added required just for frontend check, nothing more. I'll also open a PR which will check these attributes on server before publishing the event.

Edit: I'll also check that saving the event as a draft works if any of these info is missing.

@kushthedude
Copy link
Member

@shreyanshdwivedi Ticketing Toggle was already done in #2518 , waiting for @CosmicCoder96 to reopen the pr

@shreyanshdwivedi
Copy link
Member Author

@kushthedude okay. I'll remove this part from my PR :)

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented May 16, 2019

@kushthedude I waited for a day but I don't see any update on your PR related to the toggle of ticketing feature. Can I please include that in my PR as I'll be needing it for few checks? I feel it'll be better if all the checks are in a single PR

@kushthedude
Copy link
Member

kushthedude commented May 16, 2019 via email

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented May 17, 2019

So few updates here. I've commented out ticketing toggle code and code related to option to show events from other platforms using a URL for now (as they can be implemented later).
I've disabled the Publish button if it has no locationName or no tickets.

  • Check for Name is already there on frontend. I'll add a check on schema too
  • For tickets, we cannot simply raise error message from server if user has no tickets associated because save function on tickets is called after event is saved to db (as it requires event-id).
  • Location was already being checked to disable/enable Publish button and there is check on server too.
  • Checks were already there for Dates on server and they are working fine. There is a glitch here. If we delete the start_time or end_time, the frontend date-picker is giving a value to it rather than passing null to server which is why no error message is raised here.

Also, I'll have to comment out the code blocks related to is_billing_enabled from events schema as it'll not be used after this PR gets merged. I'll open a child issue related to this on server.
@uds5501 @kushthedude @ritikamotwani your views?

@shreyanshdwivedi
Copy link
Member Author

@uds5501 @pradeepgangwar can you please review this?

@pradeepgangwar pradeepgangwar self-requested a review May 19, 2019 20:42
@shreyanshdwivedi
Copy link
Member Author

So few updates here. I've commented out ticketing toggle code and code related to option to show events from other platforms using a URL for now (as they can be implemented later).
I've disabled the Publish button if it has no locationName or no tickets.
Check for Name is already there on frontend. I'll add a check on schema too
For tickets, we cannot simply raise error message from server if user has no tickets associated because save function on tickets is called after event is saved to db (as it requires event-id).
Location was already being checked to disable/enable Publish button and there is check on server too.
Checks were already there for Dates on server and they are working fine. There is a glitch here. If we delete the start_time or end_time, the frontend date-picker is giving a value to it rather than passing null to server which is why no error message is raised here.
Also, I'll have to comment out the code blocks related to is_billing_enabled from events schema as it'll not be used after this PR gets merged. I'll open a child issue related to this on server.

@CosmicCoder96 @pradeepgangwar @ritikamotwani your views?

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.

@shreyanshdwivedi Remove the merge conflicts and we will see about checks later , Let it get merged for now 👍

@iamareebjamal
Copy link
Member

This check should be on server.

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal please refer to #2926 (comment).
Server is already having checks for location, dates. I'll add a check for name on server.
For tickets we cannot simply add a check on server because tickets are sent after the event is saved ( as tickets need event ID ).
Overall, in this issue we can only add checks for name.

@iamareebjamal
Copy link
Member

I'm talking about having no tickets. The issue is about event being published without tickets. It's not about it not being saved on the server without tickets

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented May 30, 2019

@iamareebjamal when I researched on this issue earlier, I found out this -

For tickets, we cannot simply raise error message from server if user has no tickets associated because save function on tickets is called after event is saved to db (as it requires event-id).

If tickets are checked after event is saved, it'll be of no use. Can you think of any work-around here?

@iamareebjamal
Copy link
Member

You have the wrong idea of event being published. Event can be saved but it will not be published. It'll be draft event. I need to be able to create a draft event without tickets. It definitely should be saved in the DB. It just can't be published

@shreyanshdwivedi shreyanshdwivedi changed the title [WIP] feat: allows to publish event only when event has relevant info fix: hides 'use ticketing system' to prevent spam Jun 3, 2019
@auto-label auto-label bot added the fix label Jun 3, 2019
@shreyanshdwivedi shreyanshdwivedi changed the title fix: hides 'use ticketing system' to prevent spam [WIP] fix: hides 'use ticketing system' to prevent spam Jun 3, 2019
@auto-label auto-label bot removed the fix label Jun 3, 2019
@shreyanshdwivedi shreyanshdwivedi changed the title [WIP] fix: hides 'use ticketing system' to prevent spam fix: hides 'use ticketing system' to prevent spam Jun 6, 2019
@auto-label auto-label bot added the fix label Jun 6, 2019
@shreyanshdwivedi
Copy link
Member Author

@uds5501 @kushthedude please review

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.

@shreyanshdwivedi Can you please hide the Ticketing Module in Admin/Settings and set it to True for now , This would be better as we want to use the ticketing system always ?

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jun 6, 2019

@kushthedude the admin modules serves other purpose. We should not remove it IMO.
It provides admin authority to stop purchasing of any tickets on his/her discretion

Copy link
Contributor

@uds5501 uds5501 left a comment

Choose a reason for hiding this comment

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

LGTM

@abhinavk96 abhinavk96 merged commit fc3c2f8 into fossasia:development Jun 11, 2019
@shreyanshdwivedi shreyanshdwivedi deleted the publishEvent branch August 20, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide option "use ticketing system" to prevent spam and require tickets to make event live

6 participants