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

feat: implementation of post and put method on alerts #136

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

yokwejuste
Copy link
Contributor

No description provided.

@yokwejuste yokwejuste changed the title feat: implementation of post method on alerts feat: implementation of post and put method on alerts Sep 10, 2022
Copy link
Member

@Sanix-Darker Sanix-Darker left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, but I really have some concerns about how it has been done!

@yokwejuste
Copy link
Contributor Author

Thanks for this PR, but I really have some concerns about how it has been done!

Hey @Sanix-Darker can you have a look at this once more?

@@ -18,8 +18,6 @@
app.config.from_object(DevConfig)

cors = CORS(app)
Copy link
Member

Choose a reason for hiding this comment

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

I was just asking to add comments here telling the reason you added those, not remove them 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, i do prefer it like that too,

We're still in development so, warnings are not that way hurting. :)

Comment on lines +74 to +75
db.session.query(Alert).filter(Alert.id == alert_id).update(request.json)
db.session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

Much better !
Did you tested it to be sure everything was working fine ?

Copy link
Contributor Author

@yokwejuste yokwejuste Sep 12, 2022

Choose a reason for hiding this comment

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

Yeah, i did the testing.

In fact I tested the post and it works well, but as for the put, i don't know how to target the id of the alert, cause the alert is parse as a get argument /alerts?id=45

How can i get the id and send it to my put method

....
   def put(self, alert_id):
         .....
        ......

Help please 🙏

Copy link
Member

Choose a reason for hiding this comment

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

you can get access to it by using the request's arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can I do this sir? please a reference or a hint to illustrate this?

Copy link
Contributor Author

@yokwejuste yokwejuste left a comment

Choose a reason for hiding this comment

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

Yet I'm still facing an issue!

At the level of the post of lat and long, there's an issue, I'm unable to retrieve the lat and long value from the district_id, since this data is stored on the district table.

**request.json,
**{
"date": datetime.datetime.now().date(),
"begin_time": datetime.datetime.now().time(),
Copy link
Member

Choose a reason for hiding this comment

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

@yokwejuste good job, i have some questions
1 - why is not the begin and end time not comming from the user's inputs

Copy link
Member

Choose a reason for hiding this comment

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

2 - is it not better to check the user input and not directlly trust it(at least for the foreignkey)??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yokwejuste good job, i have some questions
1 - why is not the begin and end time not comming from the user's inputs

Normally an alert starts when the user tell's us about.

Should we get the start time from the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2 - is it not better to check the user input and not directlly trust it(at least for the foreignkey)??

Here, I'm not getting you,

Can you please be more explicit?

Copy link
Member

Choose a reason for hiding this comment

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

About the start time i'n nor sur we can first live it like it is

for the question 2.
a user send for example send 0 for a string as city_id(in bot case they are invalid because the ciry_id should be the id of and existing city) so we have to manually check en validate inputs before saving it in the database

Comment on lines +74 to +75
db.session.query(Alert).filter(Alert.id == alert_id).update(request.json)
db.session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

you can get access to it by using the request's arguments

@yokwejuste yokwejuste requested review from Zaker237 and Sanix-Darker and removed request for Zaker237 and Sanix-Darker September 18, 2022 07:17
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.

3 participants