-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adds clubs models #86
Conversation
Status: Blocked ❌Issues blocking this PR:This comment was automatically written by the Blocking Issues bot, and this PR will be monitored for further progress. |
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.
Awesome work, really. This is a lot in one go but it's all very consistent. I know I left a LOT of comments, but that's because it's really ready to go!
One more general point, you have to assume that pretty much all database and service calls will be a Future
, so you have to await
them. You already made each function async
, so now you should just have to put the word await
before each Services.instance.database...
. I didn't put it in the comments since there are a lot of them, but see how it goes.
// TODO: implement init | ||
throw UnimplementedError(); |
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.
Some pseudo-code (like what you would do, maybe in comments) would be nice here instead.
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.
What should I put there though? because I don't think we need anything in init() for Club Admins.
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.
So maybe this doesn't need to be a data model (ie, it shouldn't be included in the Models
collection). You can remove this function and extends Model
. Then it'll just be a view model -- something the UI can use to make specific actions easier, but doesn't need to provide the underlying data.
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
Co-authored-by: Levi Lesches <[email protected]>
For the record, here are the assumptions this PR makes: abstract class HybridClubs { // Services.instance.database.clubs
Future<List<Map>> getAll();
Future<void> register(String id, String email, Map json);
Future<void> unregister(String id, String email, Map json);
ClubAdmin admin;
}
abstract HybridClubAdmin { // Services.instance.database.clubs.admin
Future<void> upload(String id, Map json);
Future<void> delete(String id);
Future<void> approve(String id);
Future<void> postMessage(String id, Map json);
Future<void> removeMember(String id, String email);
Future<void> setAttendance(String id, Object attendance);
Future<void> modifyMeeting(String id, DateTime old, DateTime? new);
} On the data side:
|
|
Blocked by #31, #34
Fixes #37