At first I thought I had to check whether a member is active or not (member__status='member') but I don't think it make sense.
WDYT?
List the emails of members that have subscribed to the given offer.
We also make sure that the OfferSubscription hasn't been resigned.
Example :
> python manage.py per_offer_members_email awesome_offer 2>/dev/null
mail1@coin.net
mail2@coin.net
At first I thought I had to check whether a member is active or not (member__status='member') but I don't think it make sense.
WDYT?
I'm ok with integrating this command, but… We will now have 3 commands with the same output format :
members_email
per_offer_members_email
subscribers_email
… Maybe we should merge them into one single command with several --filter-options.
What do you think ?
## It works
I tested :-)
## A suggestion
- [x] filter by service ref and/or ID, which are supposedly more stable than the service names that could change
## In the details…
- [x] should be in the offers app
- [x] unused import :
```
from coin.offers.models import Offer
```
- [x] respect [the import order from pep8](https://www.python.org/dev/peps/pep-0008/#imports)
- [x] use `args` or [option_list](https://docs.djangoproject.com/en/1.7/howto/custom-management-commands/#django.core.management.BaseCommand.option_list) to handle the arg
## Taking a broad approach…
I'm ok with integrating this command, but… We will now have 3 commands with the same output format :
- `members_email`
- `per_offer_members_email`
- `subscribers_email`
… Maybe we should merge them into one single command with several `--filter-options`.
What do you think ?
filter by service ref and/or ID, which are supposedly more stable than the service names that could change
I agree, fixed.
should be in the offers app
fixed.
unused import : from coin.offers.models import Offer
I'm using it now.
respect the import order from pep8
fixed.
use args or option_list to handle the arg
I'm not sure I understand how to use args correctly. Do you mean I should set it like the help variable (see updated diff)?
Since the command now uses offer_id, I've slightly changed how the args are parsed.
Thoughts?
One command to rule them all
I agree if we keep it simple, that is, if we don't try to add complex filter options like --before/after date.
I'll have a look at those commands and propose a patch.
Btw, where should the script go? in the offer or in the member app? :p
## Fixes
- filter by service ref and/or ID, which are supposedly more stable than the service names that could change
I agree, fixed.
- should be in the offers app
fixed.
- unused import : from coin.offers.models import Offer
I'm using it now.
- respect the import order from pep8
fixed.
- use args or option_list to handle the arg
I'm not sure I understand how to use `args` correctly. Do you mean I should set it like the `help` variable (see updated diff)?
Since the command now uses `offer_id`, I've slightly changed how the `args` are parsed.
Thoughts?
## One command to rule them all
I agree if we keep it simple, that is, if we don't try to add complex filter options like --before/after date.
I'll have a look at those commands and propose a patch.
Btw, where should the script go? in the offer or in the member app? :p
I'm not sure I understand how to use args correctly. Do you mean I should set it like the help variable (see updated diff)?
Since the command now uses offer_id, I've slightly changed how the args are parsed. Thoughts?
That's fine, you can do better/cleaner/shorter, but that's with more recent versions of Django. My bad.
I agree if we keep it simple, that is, if we don't try to add complex filter options like --before/after date.
They may come later (I mean, as long as they are optional…)
I'll have a look at those commands and propose a patch.
Cool. So, for this very PR, I let it unmerged for now ?
Btw, where should the script go? in the offer or in the member app? :p
member as we display primarily display… members :-)
I'm pretty sure we could refine if we want to have different options if some apps are installed, but IMHO, that would be a nice thing, not an obligation.
> I'm not sure I understand how to use args correctly. Do you mean I should set it like the help variable (see updated diff)?
> Since the command now uses offer_id, I've slightly changed how the args are parsed. Thoughts?
That's fine, you can do better/cleaner/shorter, but that's with more recent versions of Django. My bad.
> I agree if we keep it simple, that is, if we don't try to add complex filter options like --before/after date.
They may come later (I mean, as long as they are optional…)
> I'll have a look at those commands and propose a patch.
Cool. So, for this very PR, I let it unmerged for now ?
> Btw, where should the script go? in the offer or in the member app? :p
member as we display primarily display… members :-)
I'm pretty sure we could refine if we want to have different options if some apps are installed, but IMHO, that would be a nice thing, not an obligation.
List the emails of members that have subscribed to the given offer. We also make sure that the OfferSubscription hasn't been resigned.
Example :
At first I thought I had to check whether a member is active or not (member__status='member') but I don't think it make sense.
WDYT?
It works
I tested :-)
A suggestion
In the details…
from coin.offers.models import Offer
args
or option_list to handle the argTaking a broad approach…
I'm ok with integrating this command, but… We will now have 3 commands with the same output format :
members_email
per_offer_members_email
subscribers_email
… Maybe we should merge them into one single command with several
--filter-options
.What do you think ?
Fixes
I agree, fixed.
fixed.
I'm using it now.
fixed.
I'm not sure I understand how to use
args
correctly. Do you mean I should set it like thehelp
variable (see updated diff)?Since the command now uses
offer_id
, I've slightly changed how theargs
are parsed. Thoughts?One command to rule them all
I agree if we keep it simple, that is, if we don't try to add complex filter options like --before/after date.
I'll have a look at those commands and propose a patch. Btw, where should the script go? in the offer or in the member app? :p
That's fine, you can do better/cleaner/shorter, but that's with more recent versions of Django. My bad.
They may come later (I mean, as long as they are optional…)
Cool. So, for this very PR, I let it unmerged for now ?
member as we display primarily display… members :-)
I'm pretty sure we could refine if we want to have different options if some apps are installed, but IMHO, that would be a nice thing, not an obligation.
#99 est fermé, te voilà donc débloqué :-)
Cf #97