Skip to content
This repository has been archived by the owner. It is now read-only.

Adding e-commerce logic #48

Merged
merged 31 commits into from Oct 26, 2015
Merged

Adding e-commerce logic #48

merged 31 commits into from Oct 26, 2015

Conversation

@benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Sep 19, 2015

This is a work in progress. Integrating Pawel's work with the simple "webshop" but not quite there yet...

  • The payments and webshop apps should be merged, there's no idea having a hard dependency point from webshop to payments anyways.
  • All users should have accounts (as in accounting accounts)
  • A user profile can have multiple accounts
  • A user profile can have many memberships that can be renewed and have different prices and types. Memberships can decide on whether a user is eligible to do certain things like ordering specific products and signing up for shifts.
  • We need to track Orders, Invoices etc.

Whether or not this should integrate with an existing eCommerce system like Oscar is very difficult IMO.

For instance, "the bag of the week":

  • Has to be ordered 7 days ahead, before an exact deadline
  • Can also be ordered as "the bag in 3 weeks"
  • Has to be tracked as an individual product in order to count
    • How many bags were ordered
    • At which coop should they be picked up
    • Un-ordered bags, i.e. bags for sale at each location
  • Order can possibly be cancelled before the deadline?
  • When do we charge money from people's accounts?

Lalala, lots of questions that I think beg to have a customized system rather than seeing this as conventional online shopping :) So I'm saying that it was good Pawel started a custom solution, at the moment it looks like an attempt to invent the wheel, but I'm gonna try in this PR to prove why we need it :)

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Sep 19, 2015

I've renamed "webshop" to "market" to reflect the over all idea that there will be a kind of trade going on... it's the best way I can imagine that we are actually creating something that's not a conventional webshop :)

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Sep 19, 2015

Some more on what I meant with "kind of trade":

We have an idea that at some point this system will handle more than people buying "this weeks bag". Farmers will actually sell an amount of potatoes to be redistributed to coop members.

If this is where we want to end up, I think it should be reflected in the way that we build the "ecommerce".

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Sep 19, 2015

@valberg @andreaslloyd @sepow @cheng81 @pawciobiel

So I've gone ahead and done this thing with the "market" application in order to handle both payments and orders. Potentially, it will also handle accounts and invoices.

Any inputs on this direction?

(the code is not where it should be, but I think it can be merged in order to open up the task to more people)

$(MAKE) -C docs clean
$(MAKE) -C docs html
open docs/_build/html/index.html
xdg-open docs/_build/html/index.html

This comment has been minimized.

@valberg

valberg Sep 20, 2015
Contributor

xdg-open is a linux thing (open is mac os x).

I suggest we let the developer open the docs themselves :)

This comment has been minimized.

@benjaoming

benjaoming Sep 20, 2015
Author Contributor

Aaaah, that way :) Are you saying there are devs on OSX!? :D I kept getting an error on open... I'll just remove the whole line yes.

@andreaslloyd
Copy link
Member

@andreaslloyd andreaslloyd commented Sep 20, 2015

Good stuff! I can't really comment on the code, but on a conceptual level, I agree with having a Market app rather than just a webshop. I'd like to talk a little more about how the Marketplace should be set up. Initially, the idea was to have two separate apps – one for members ordering/paying and another for co-op purchasers and farmers to coordinate and plan bulk orders and distribution. I still think it is important to separate the two to some extent. But I suppose it would be neat if the Market could handle accounting/invoices etc. for both.

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Sep 20, 2015

I agree that this separation is desirable in some form, but I'm seeing a lot of connections between the logic of coop members' data and farmers' data... something like:

Farm has 300 kg of eggplants => Coop creates bags => Members order bags with eggplants.

I'm not exactly sure of the consequences. But the previous separation between payment and webshop didn't have any real explicit means of saying how payments were separate from the webshop. I think we can try to create a separation, but I don't know if "paying for stuff" is enough.

So the idea of having one big market function in ways that we define as "the ideal direct trade between farms and coops" would result in a singular application logic IMO.

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 3, 2015

Okay membership.models.profile.Account has been consolidated.

I also created membership.models.permissions with the intention to use boolean fields and make all permissions explicit. Sorry about ditching the Django permissions framework in favor of this, but the decision had kind of already been taken, so I just slided gradually over to a department/account level of attaching decisions to a userprofile.

So you can say:

  • This user has this set of predefined permissions on this department
  • This user has this set of predefined permissions on this account

Each permission set is a row of booleans! So for instance "team link" would have a set of booleans like "can delete vegetable bag" or "can change other people's permissions in this department".

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 4, 2015

Would be great to have this PR reviewed again. I worked in a separate branch refactoring membership and then rebased into this branch. So the new changes are some of the first commits in here.

Anyways, because there's so many changes it won't be feasible to have this lying around for too long :)

# TODO: This is not the way to do it, we should have delivery dates specified
# on the products themselves -- they are not to be decided by the member
# but are preconfigured
delivery_date = models.DateField(null=False, blank=False,

This comment has been minimized.

@valberg

valberg Oct 5, 2015
Contributor

Remove it then? :) (referring to the comment above)

@valberg
Copy link
Contributor

@valberg valberg commented Oct 5, 2015

I was quite surprised to see the membership part being made into an user only being able to be part of one account. The first hackday (IIRC) we decided that an user should be able to be part of for instance a household and a company. Example: Laura is a member of kbhff nørrebro through her commune and kbhff amager through her sports club. This means that Laura can take shifts for either of these accounts - what this PR does is to make this not possible since Laura would only be able to part of one account, unless she had multiple logins (which we don't want).

I've talked to @benjaoming about it yesterday, and I think we came to the conclusion that we're going back to that setup?

@@ -162,7 +158,7 @@ def test_send_invitation(self):
expected = 'Invitation has been send to {}'.format(invited_email)
self.assertContains(response, expected, 1, 200)

self.assertEqual(len(mail.outbox), 1)
self.assertEqual(len(mail.outbox), 1) # @UndefinedVariable

This comment has been minimized.

@valberg

valberg Oct 5, 2015
Contributor

If it is okay with you, I would just love if we keep those editor specific things out of the codebase? Pretty please 😍

This comment has been minimized.

@benjaoming

benjaoming Oct 5, 2015
Author Contributor

They're really really helpful because I usually spot many errors by having a static check enforced through-out the codebase (especially from people who don't use a static check). It's because mail.outbox doesn't resolve, but I think it's like the only place in the whole codebase there's something like that :)

This comment has been minimized.

@benjaoming

benjaoming Oct 5, 2015
Author Contributor

The helpful part is that I keep going back to this line because it has an error... so by putting that innocent little comment there, it switches off the error. I actually added it manually :)

This comment has been minimized.

@valberg

valberg Oct 5, 2015
Contributor

It is still hideous and clutters up code 😝 Also, this is a test, it will break if .outbox doesn't exists :)

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 5, 2015

I didn't say the part of having one account per profile is a deal breaker or anything :) I just made a really nice simplification to get around some of the half-finished work with payments and stuff that I've been spending loads of time refactoring into the market app.

As for sharing stuff, the current status is: It's possible to share an account. Departments are not the same as coops of course, so a profile or account could move freely between those.

The said example where a profile can switch between using a sport club's account and membership and a private one isn't supported in the current state.

I'm not saying that we shouldn't, just that implementing it half-way generated another problem elsewhere (is_active_account_owner) so I removed it so we can start from fresh.. either re-implementing it or ditching it.

@valberg
Copy link
Contributor

@valberg valberg commented Oct 5, 2015

Yeah I get that - but if we keep on refactoring basics like this we'll keep on going forever. We discussed the models for profile/account/department at length the first hackday (sad that we didn't write it down) and there we opted for the more flexible approach where users can be members of multiple accounts (and share as many accounts as is necessary).

I'm not "fighting" simplicity (I see what we have now as quite simple 😄 ), I only want us to progress, instead of changing each others work all the time 😁

In the end it all comes down to if @andreaslloyd says OK to users only being able to have one account 😉

@valberg
Copy link
Contributor

@valberg valberg commented Oct 5, 2015

Wait.. Just reread and got puzzled by I didn't say the part of having one account per profile is a deal breaker or anything :) currently this is not the case, currently there is a many-to-many relationship between Account and Profile (defined here https://github.com/kbhff/eggplant/blob/master/eggplant/membership/models/account.py#L47-L50).

And this PR changes that into a Account has many Profiles, but a Profile only has one Account (defined here https://github.com/kbhff/eggplant/pull/48/files#diff-8a5daddbe9e5cdbb1837959b0f0d7a6aR22).

So maybe this needs clarifying somewhat ☺️

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 5, 2015

The simplification argument goes like this:

Before a profile could have many accounts. So in many cases we will have to say "the user has 0, 1 or more accounts" and then treat the results of that query. This was why is_active_account_owner was added as a sort of symptom.

Now, we know that a Profile has 1 account. Except for the null=True part, which I'm really also wanting to fix if I should defend it any further :)

But let me get back to coding, I can make a re-affirmed suggestion, re-adding the multiple account relation, especially now that is_active_account_owner has simply been removed :)

One thing we haven't discussed is the relationship between accounts and memberships which me and @andreaslloyd talked about. The issue here is more about definition and sane data: An account does not expire. For instance, if your membership is expired and a credit card transaction is rejected, you still have an account that's active. Memberships are more like individual objects that track the subscription (kontingent) of a member. But it's on the TODO :) I'm changing the PR tasks, hoping I have time to include it here.

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 5, 2015

Btw thanks for reviewing :) I've changed the tasks of the description now to reflect the intentions.

@andreaslloyd
Copy link
Member

@andreaslloyd andreaslloyd commented Oct 6, 2015

Hey, good discussion. Also good to have it online for future reference. :-)

+1 for re-adding the multiple account relation. We have discussed this in
detail previously, and opted for the flexibility.

I'm fine with the model that memberships can be individual objects that
track subscription. It probably needs to be simplified when presented on
the user-side, but if it is a helpful distinction for the coding, it's all
good for me. :-)

On Tue, Oct 6, 2015 at 12:12 AM, benjaoming notifications@github.com
wrote:

Btw thanks for reviewing :) I've changed the tasks of the description now
to reflect the intentions.


Reply to this email directly or view it on GitHub
#48 (comment).

Andreas Lloyd

web: www.andreaslloyd.dk
mail: andreas.lloyd@gmail.com
phone: +45 3025 3049

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 21, 2015

"Many accounts" feature just came back, but now without the roles field because this is handled by membership.models.permissions in a more generic way.

@pawciobiel
Copy link
Contributor

@pawciobiel pawciobiel commented Oct 26, 2015

@benjaoming would it make sense to merge this one in and carry on the remaining work in separate branches?

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 26, 2015

@pawciobiel yes, I've just been sitting on it until someone was actively blocked by its existence :)

One of the things it does is to recreate all migrations, so beware of that. This was also the main reason why I didn't merge it, because I wanted the Membership model to be added.

See you l8r today?

@valberg
Copy link
Contributor

@valberg valberg commented Oct 26, 2015

Merging is 👌 with me :) @benjaoming the hackday is next monday http://www.meetup.com/Eggplant/events/226229531/ ;)

@valberg
Copy link
Contributor

@valberg valberg commented Oct 26, 2015

Also, as long as we have not deployed this anywhere migrations are less important IMO. I expect us to have to do a squash somehow when we release the first version 😄

@pawciobiel
Copy link
Contributor

@pawciobiel pawciobiel commented Oct 26, 2015

@benjaoming I was thinking about using https://github.com/lambdalisue/django-permission but wasn't sure if you need something a bit more fancy, I guess you did since you added your own permissions.

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 26, 2015

Yes, sorry about the migration reset, I just kind of got carried away by the ideal of a perfect and clean codebase :D

@pawciobiel
Copy link
Contributor

@pawciobiel pawciobiel commented Oct 26, 2015

@benjaoming I think it is good to have order/payment id truly unique; what was your use-case for changing this to auto incremented integer? Also, out of curiosity, was there anything wrong with UserProfile.sex field which may have suggested we should change it to integer or was that your personal preference?

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 26, 2015

@pawciobiel using PositiveSmallInteger is the closest we can get to enums in Python :) No reason to store text strings for that. It doesn't change behaviour.

@valberg
Copy link
Contributor

@valberg valberg commented Oct 26, 2015

I'm also more in favor of CharField for something with choices. It makes debugging way easier since you don't have to remember what the numbers map to when for instance looking in the database or at a JSON payload.

benjaoming added a commit that referenced this pull request Oct 26, 2015
@benjaoming benjaoming merged commit 2873c61 into kbhff:master Oct 26, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 26, 2015

Garh, sorry so you are suggesting to make CharField -- okay, adding an issue for that then.

@benjaoming benjaoming mentioned this pull request Oct 26, 2015
@pawciobiel
Copy link
Contributor

@pawciobiel pawciobiel commented Oct 27, 2015

@benjaoming There closest you can get to Enum in Python is Python Enum PEP-435
I just wanted to suggest/ask if we could keep change to minimum and slow down with refactoring a bit please?

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Oct 27, 2015

@pawciobiel there's already an issue for this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants