HasteDesign / Registrations-for-WooCommerce

Add a registration product type to your WooCommerce installation.
GNU General Public License v2.0
38 stars 16 forks source link

Master #4

Closed Shirkit closed 7 years ago

Shirkit commented 7 years ago

Here's the PR with two separate commits attacking two different issues. Hopefully someone can help me find a solution to the notice and translation issue.

Shirkit commented 7 years ago

The commit b1fa5e47789292d0e705924d833232eaf4b60d05 does not address properly the case for range events. I'll be trying to solve this later. PR is not ready for merge at this point, sorry I missed this. Well, it was not ready yet anyways since it needs to solve the two remaining issues still.

Shirkit commented 7 years ago

Fixed the previous issue, now it's left to these 3:

allysonsouza commented 7 years ago

Hi @Shirkit,

Thanks for the contribution! I'll be great features for registrations. Testing the features I see:

But thanks again, great work, I'll see later the date prettify.

Shirkit commented 7 years ago

Ok @allysonsouza,

I think the PR is now closer to finished, but maybe you should take a look. There's still a few things to keep in mind:

  1. the translations are not working yet for some reason and I can't understand why.

  2. further testing needs to be done (I've tried to cover all cases but I may have missed something)

  3. the post meta for the orders have been completely changed. This was required to to the report functionality, but this breaks with the current version of the plugin.

So, to solve 3, I thought of maybe, when the plugin updates, it would need a database update so it fixes to the proposed standard.

Also, feel free to comment if 3 should be dropped in the first place.

allysonsouza commented 7 years ago

Hi @Shirkit,

Firstly, I've merged your pull request, but just up to Prevent registration from past events, thanks, that is a great missing feature on the plugin, and format the dates in every page it's basic and great (when I started the plugin there's no hooks in WooCommerce to that).

I think the report functionality is great too, but we can see it in other Pull Request, and discuss the break of compatibility.

I gave a look into the new prevent past dates and I didn't understand, in which situation absolute prevent past date to all variations dates would be usefull? I think maybe just relative prevent past dates is necessary. (Waiting your point of view to make that change).

I'll format the code and strings according to WordPress Coding Guide and soon we can release a minor version with that changes.

Thank you very much!

Shirkit commented 7 years ago

For some reason, the message I wrote a week ago got lost in the way!

So, I think it's fair to create a separated area for the report since it breaks compatibility between versions. I also think it should be looked into, but the current way the data was being stored was barely accessible.

The prevent past dates I did on this way because I could come up with a few cases in which the plugin could be used:

That's why I implemented like that, my client actually has the second event being held, so I ended up developing the code to cover all of those 3 cases, which makes sense for me.

You want to create the new PR or want me to create it?

allysonsouza commented 7 years ago

Hi @Shirkit,

Github sometimes makes weird things with our messages >.<

Report I liked what you have started in the report functionality, but I thinking about a great change in how the order meta (participant name, email and any possible custom field they could have) are stored. The basic comma separeted values are not scalable.

Prevent Past Dates About prevent past dates, now I understand your logic, but I think the multiple date is for one instance of the event, if the event will sell tickets for days separetely, they can create various single dates (and one multiple date variation for all days tickets).

Shirkit commented 7 years ago

Hello @allysonsouza

Indeed the case for the Prevent Past Dates can be simplified, I always think what the user would expect. Just a matter of taste if want to keep that.

Now the report functionality you are right, my solution was pretty flawn.

I see you've started working (I totally missed that) on the plugin, what you need to finalize the next build so I can assist you?

allysonsouza commented 7 years ago

Hi @Shirkit,

I'm just testing if the new way to store order meta (serialized) will break any compatibility, but I think it will not. Maybe it's ready for the 1.0.7, and in the next release the report functionality (and also it's necessary to make the plugin rready to WooCommerce 3.0, that I'll have great changes).

Thanks for all the support!

Shirkit commented 7 years ago

So @allysonsouza ,

Cool. For what I've seen, you're only missing the report functionality, since everything else seems to be done. There's one thing I don't understand though: why you create Wordpress users for the running website for the people that registered for the event? Is that only to allow easier access in the reports?

I can't grasp why it would be needed, specially with random passwords and their emails are never pinged to tell they have an account. That information, at least in my opinion, should be stored along the order (as it's currently being), and nowhere else. Filling the website with hundreds of users that will never login in is something I can't understand, specially since the exact same data is being replicated. For some types of events, users would be granted access, let's say, to a restricted area of the website, or a protected download, but that's a very edge case to justify creating the users without telling them for every single event.

Either way, if you are not doing the Reports, and you're done with the way the meta data is being store, I will re-fork and begin implementing them, this time a bit better.

I'm running my own version of the plugin, but I'm looking forward to have the next version working on my clients website. A little database migration and I'll be good!

allysonsouza commented 7 years ago

Hi @Shirkit,

The user creation was a feature from the project that originate that plugin, my intention is to make optional to create a user for participants or not, creating a plugin options page or maybe just adding some hooks for that (the second option looks better for me at the moment). The same with Groups plugin integration.

If you can work on Reports I'll really appreciate it, I started locally but I wasn't able to understand some aspects of the necessary code to generate the reports.

allysonsouza commented 7 years ago

@Shirkit I've added that questions to issues, and assigned to milestone 1.0.7. The Reports I've added to 1.1.0 (I don't know if you know SemVer, but I'm trying to follow your rules: http://semver.org/).

The 1.0.7 I'll try to release on sunday, there's some users needing the hooks to add custom participant fields and they are waiting for too long.

Shirkit commented 7 years ago

Ok, let's migrate the conversation to the proper issues then. I'll start working on the Reports. Most likely I'll work on the others as well, since I have the day off today and I need to finish it for the client. It's a pleasure to get to help on the project!