Skip to content
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

registered project blueprint and created update project route #40

Closed
wants to merge 17 commits into from

Conversation

CurtisMIT
Copy link
Collaborator

Summary

I've registered the 'project' blueprint and created a route to update a project based on the specified uuid.
The defined function is subject to change, as it is dependent on how the 'Photo' and 'Project' models are defined and how photos of a project should be updated.

Details

  • I plan to add a serialize method in the 'Project' model similar to how Celine did in her examples, as it would help a lot when returning the data as JSON.
  • As the discussion for this issue mentioned, we are treating zero updates to a project as an error (i.e. if we are directed to this route without any changes proposed for the specified project, we will return an error)
  • We have not discussed the way of updating the photos. As a 'Project' has many photos, would we allow a user to add new photos to the ones currently stored in the servers, or would we allow users to wipe the currently stored photos in the server and simply add the new ones. I'm leaning towards the first option, would love to hear suggestions.

Test Plan

I plan to write unit tests for this route after dependent issues are closed and corresponding pull requests are merged.

Related Issues

@celinehuang
Copy link
Member

looks good otherwise, good PR description with lots of context! we will wait for your other PRs and then it's probably good to merge it in

Celine Huang and others added 2 commits November 23, 2020 19:00
* Initial project domain model
* Reformatted code
* Update Project Model
1. Implemented UUID
2. deleted an empty line
3. Ran black on the model.py again to ensure formatting consistency
@@ -49,4 +49,8 @@ def create_app(config_name):

app.register_blueprint(note_blueprint, url_prefix="/note")

from .project import project as project_blueprint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch lol my bad

year = data.get("year")
name = data.get("name")
# should we change the name of the attribute to project_type instead
type = data.get("type")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no preference on whether to change it or not, although you could just do project_type = data.get("type") instead (<- not sure if this is what you mean) since that wouldn't affect the model. Up to you though, I personally think it'd be ok - @peterghrong thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

project_type would be better in my opinion, since there is also the contact_type, Idk how I'm only reading this now. Apologies !

ahmad675 and others added 12 commits December 1, 2020 00:08
* Added Donation domain model class

Co-authored-by: aqghawa <[email protected]>
* Creating a general model for Contact Type

* Running black locally for testing purposes.

* Adding necessary changes for ContactType Domain Class.
* Added MuUser domain model class

* Changed id and class names to match domain model

* Removed Removed everything related to authentication, this includes imports, http_auth, and password properties. Added lazy=true.

* Removed an extra comma

* Black format

* Removed password_hash

Co-authored-by: aqghawa <[email protected]>
* Fix models.py to follow Python naming conventions

* Oops remove more note association

* Pluralize table name

* Fix names to follow Python conventions

* Small fixes
* Add project serializer

* Remove pluralization
@CurtisMIT
Copy link
Collaborator Author

Added some units tests to make sure everything works.

@CurtisMIT
Copy link
Collaborator Author

Closed as #85 was created to remove git issues

@CurtisMIT CurtisMIT closed this Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to edit a project
5 participants