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

Equipment post is modified to use the database. #89

Merged
merged 6 commits into from
Jun 10, 2021

Conversation

Voursstrreds
Copy link
Collaborator

Equipment post may need more changes according to the given location input. It may change.

I modified the function to add data to the database, not the ordinary lists.

According to the review, we imported the libraries that we use in the post equipment.
Copy link
Collaborator

@ahmetnecipg ahmetnecipg left a comment

Choose a reason for hiding this comment

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

Necessary database tables and functions need to be imported from .dbinit file. In this case Equipmentpost table needs to be imported.

@Voursstrreds
Copy link
Collaborator Author

Necessary database tables and functions need to be imported from .dbinit file. In this case Equipmentpost table needs to be imported.

I fixed this.

@Voursstrreds
Copy link
Collaborator Author

Now this function works.

Copy link
Contributor

@supiket supiket left a comment

Choose a reason for hiding this comment

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

The imports and request.json null checks are on-point. The returned value is jsonified correctly.

Copy link
Collaborator

@ibrahimbayat ibrahimbayat left a comment

Choose a reason for hiding this comment

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

Function is very clear and understandable. It controls all the possible missing parameters and posting accordingly. Finally it returns appropriate json format. I think this function can satisfy the needed functionality.

@Voursstrreds
Copy link
Collaborator Author

Necessary database tables and functions need to be imported from .dbinit file. In this case Equipmentpost table needs to be imported.

Can you review this now again?

@Voursstrreds
Copy link
Collaborator Author

The session statements were forgotten, they were added.

Copy link
Contributor

@supiket supiket left a comment

Choose a reason for hiding this comment

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

The function communicates with the database correctly and functions as intended.

My only (optional) suggestion is renaming the variable d1 to something like creation_date in
d1 = datetime.today()
for better readability.

Copy link
Contributor

@supiket supiket left a comment

Choose a reason for hiding this comment

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

As we discussed with @Voursstrreds on Discord:
Session = sessionmaker(db)
session = Session()
Instead of starting a new session on equipment.py with the instructions above, session can be imported from .dbinit.
This is optional since our focus is on functionality and this code works as is.

Copy link
Collaborator

@ahmetnecipg ahmetnecipg left a comment

Choose a reason for hiding this comment

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

Necessary imports were made. Approved

Copy link
Contributor

@supiket supiket left a comment

Choose a reason for hiding this comment

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

I think ownerId should be changed to ownerID.
Also the non-nullable attributes in the .dbinit file are:

  • In Post(base):
    • ownerID
    • title
    • creationDate
  • In Equipmentpost(Post):
    • equipmentType
    • websiteName
    • link

Please make sure these are all checked for bad request (code 400).

@Voursstrreds
Copy link
Collaborator Author

Voursstrreds commented Jun 10, 2021

I think ownerId should be changed to ownerID.

ownerID is changed to the form that you say. For the other one, only the creationDate is not defined exactly, and it does not need this. The function itself calculates the time every time we invoke the function.

@supiket
Copy link
Contributor

supiket commented Jun 10, 2021

You are correct, for a second I though creationDate should also be checked but of course it is initialized from datetime.today(). My mistake.

Copy link
Contributor

@supiket supiket left a comment

Choose a reason for hiding this comment

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

Approved as are no further suggestions or corrections to be made.

@Voursstrreds Voursstrreds merged commit f19e427 into development Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants