-
Notifications
You must be signed in to change notification settings - Fork 31
Allow Members to Opt-In to Active Status #147
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.
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
Conversation
@@ -0,0 +1,32 @@ | |||
import FetchUtil from "../utils/fetchUtil"; | |||
|
|||
export default class becomActive { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing an e
in becomActive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #141 it looks like the idea is to state the intent to become active, so the financial director can have a list of who wants to become active and needs to pay dues.
The current implementation just allows them to become active immediately and forces the financial director to reconcile who hasn't paid dues. Is that the desired behavior?
@audiolion When I was Financial I changed the system so that anyone who wanted to become active could, then a list of active members was sent to Housing to be charged. I would read #141 if you haven't already. |
warningText: "Becoming an active member means that you will be charged" + | ||
" dues, which are $80 per semester.", | ||
successText: "You are now an active member." | ||
}, () => { | ||
document.getElementById('coop-' + this.uid).remove(); | ||
document.getElementById('becomeActive').remove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are only hiding the button, not the entire div.
conditional/templates/dashboard.html
Outdated
|
||
{% if student and not active %} | ||
<div class="alert alert-info"><span class="glyphicon glyphicon-exclamation-sign white" style="padding-right:5px"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure an alert is the best container for this, I would go with a panel.
If you really want to bring attention to it, a colored panel it definitely an option.
<div class="panel panel-info">
<div class="panel-heading">
<h3 class="panel-title">Panel info</h3>
</div>
<div class="panel-body">
Panel content
</div>
</div>
conditional/templates/dashboard.html
Outdated
</div> | ||
{% if student and not active %} | ||
<div class="alert alert-info"><span class="glyphicon glyphicon-exclamation-sign white" style="padding-right:5px"></span> | ||
Hey there! You're eligible to become an active member, click the button below if you'd like to pay dues and become active! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, you're eligible to become an active member! Click the button below if you'd like to become active and pay dues.
CurrentCoops.semester != "Neither", | ||
CurrentCoops.uid == member.uid).first() | ||
if will_coop: | ||
dues = 80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to make the price of dues a site setting too, but I wouldn't consider it a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree, but should probably be in a separate PR/GH Issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably 5-10 minutes of work, I don't see why it shouldn't be included. You just have to add an option in the config and then import the app.config
into this file.
rit_username = re.search(".*uid=(\\w*)", member.ritDn).group(1) | ||
will_coop = CurrentCoops.query.filter( | ||
CurrentCoops.date_created > start_of_year(), | ||
CurrentCoops.semester != "Neither", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this value possible? models.py doesn't seem to indicate it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is, now that you mention it. I was modeling my query after this one in the /manage route, where they use that. I'll fix this query in a new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, Neither
was initially going to be a part of the scheme, but it was removed when we talked about how the system should work modification-wise.
@RamZallan In my initial testing, everything looks to work as intended. Just squash those fix commits (d9fa383 / bd629ae) and we should be good to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Closes #141