PeterSurda
/
inoreader2readwise
1
0
Fork 2
Code Issues Pull Requests 4 Packages Projects Releases Wiki Activity

url encoding with python #4

Open
shailaja wants to merge 1 commits from Services/part3 into main
pull from: Services/part3
merge into: PeterSurda:main
Conversation 7 Commits 1 Files Changed 2 +199
shailaja commented 2024-02-14 05:30:28 +00:00
Collaborator
Copy Link
No description provided.
shailaja added 1 commit 2024-02-14 05:30:29 +00:00
shailaja requested review from swapnil 2024-02-14 05:30:40 +00:00
swapnil commented 2024-02-14 05:44:22 +00:00
Collaborator
Copy Link

@shailaja Please include only the required changes.

Upon comparision I found that

  • Entire 'home' function has been changed, which is not reuired. Ideally, you'd only add the section to create oauth_url and pass it.
  • You're redirecting to oauth_url from within the flask. Instead you need to send the oauth_url that you've created to the login.html template via render_template method.
  • Existing comments have been removed, one of which includes the auth url of inoreader. Preserve the existing comments.
@shailaja Please include only the required changes. Upon comparision I found that - Entire 'home' function has been changed, which is not reuired. Ideally, you'd only add the section to create `oauth_url` and pass it. - You're redirecting to `oauth_url` from within the flask. Instead you need to send the `oauth_url` that you've created to the `login.html` template via `render_template` method. - Existing comments have been removed, one of which includes the auth url of inoreader. Preserve the existing comments.
shailaja added 1 commit 2024-02-14 08:00:50 +00:00
shailaja force-pushed Services/part3 from af486e5b48 to cc2357622b 2024-02-14 08:06:11 +00:00 Compare
swapnil commented 2024-02-14 12:14:50 +00:00
Collaborator
Copy Link

@shailaja changes looks fine now. Have you tested the app code with the latest changes?

@shailaja changes looks fine now. Have you tested the app code with the latest changes?
swapnil requested changes 2024-02-14 12:17:43 +00:00
app/main.py Outdated
@ -0,0 +41,4 @@
last_synced = datetime.fromtimestamp(token.get('updated_at')).strftime('%Y-%m-%d %H:%M:%S')
next_sync = datetime.fromtimestamp(token.get('updated_at') + token.get('expiration_seconds')).strftime('%Y-%m-%d %H:%M:%S')
return render_template('home.html', user_login=user_info.get('login'), user_email=user_info.get('email'), # for inoreader it's userName and userEmail
swapnil commented 2024-02-14 12:17:19 +00:00
Collaborator
Copy Link

Format this part. Does not look very nice. Maybe pass each parameter on a different line.

Format this part. Does not look very nice. Maybe pass each parameter on a different line.
swapnil marked this conversation as resolved
shailaja commented 2024-02-14 12:23:31 +00:00
Author
Collaborator
Copy Link

@shailaja changes looks fine now. Have you tested the app code with the latest changes?

yes ,but facing the same issue which is because of email

> @shailaja changes looks fine now. Have you tested the app code with the latest changes? yes ,but facing the same issue which is because of email
shailaja added 1 commit 2024-02-14 12:43:16 +00:00
shailaja force-pushed Services/part3 from 5a5d72aadc to 78ae2aa5f7 2024-02-14 12:44:59 +00:00 Compare
swapnil commented 2024-02-14 13:31:24 +00:00
Collaborator
Copy Link

@shailaja changes looks fine now. Have you tested the app code with the latest changes?

yes ,but facing the same issue which is because of email

That's not a problem. That's something Github specific. I tested by logging in and it's working but I see the last ino-app docker image was built about 8 hrs ago. That image has the lastest changes, right?

> > @shailaja changes looks fine now. Have you tested the app code with the latest changes? > > yes ,but facing the same issue which is because of email That's not a problem. That's something Github specific. I tested by logging in and it's working but I see the last `ino-app` docker image was built about 8 hrs ago. That image has the lastest changes, right?
swapnil commented 2024-02-14 13:49:55 +00:00
Collaborator
Copy Link

@shailaja I looked at the image and the latest code on the test server. The code on the test server does not match with the one in the PR. Could you test once on the latest code?
Let me know if you need my help.

@shailaja I looked at the image and the latest code on the test server. The code on the test server does not match with the one in the PR. Could you test once on the latest code? Let me know if you need my help.
swapnil commented 2024-02-17 14:42:16 +00:00
Collaborator
Copy Link

@shailaja I tested the latest code in shailja_test branch on test2 server and found that it's working fine, meaning url is generated properly. Please update this PR to include latest changes.

@shailaja I tested the latest code in shailja_test branch on test2 server and found that it's working fine, meaning url is generated properly. Please update this PR to include latest changes.
shailaja added 1 commit 2024-02-18 07:38:13 +00:00
shailaja force-pushed Services/part3 from 549cbb97fa to ec1f855261 2024-02-18 07:41:02 +00:00 Compare
swapnil commented 2024-02-20 10:36:32 +00:00
Collaborator
Copy Link

@PeterSurda this LGTM 👍

@PeterSurda this LGTM 👍
PeterSurda requested changes 2024-02-25 11:05:30 +00:00
app/main.py Outdated
@ -0,0 +24,4 @@
app.secret_key = secret_key
# https://www.inoreader.com/oauth2/auth
AUTH_URL = 'https://github.com/login/oauth/authorize'
PeterSurda commented 2024-02-25 10:52:02 +00:00
Owner
Copy Link

should point to inoreader, not github

should point to inoreader, not github
app/main.py Outdated
@ -0,0 +28,4 @@
@app.route('/')
def home():
if is_logged_in():
PeterSurda commented 2024-02-25 10:55:43 +00:00
Owner
Copy Link

this section can be extracted. I.e.

if is_logged_in():
    return main_menu():

# Generate a CSRF protection string
.
.
.
this section can be extracted. I.e. ``` if is_logged_in(): return main_menu(): # Generate a CSRF protection string . . . ```
app/main.py Outdated
@ -0,0 +35,4 @@
resp_json = resp.json()
token = resp_json['token']
user_info = requests.get('https://api.github.com/user', headers={
PeterSurda commented 2024-02-25 10:53:08 +00:00
Owner
Copy Link

should point to inoreader. Maybe define this URL as a constant?

should point to inoreader. Maybe define this URL as a constant?
app/main.py Outdated
@ -0,0 +94,4 @@
# TEST: Github OAuth - REMOVE
response = requests.post(
'https://github.com/login/oauth/access_token',
PeterSurda commented 2024-02-25 10:57:25 +00:00
Owner
Copy Link

Use a constant

Use a constant
app/main.py Outdated
@ -0,0 +116,4 @@
# REPLACE user API call with inoreader API call
# https://www.inoreader.com/reader/api/0/user-info
user_info = requests.get('https://api.github.com/user', headers={
PeterSurda commented 2024-02-25 10:57:52 +00:00
Owner
Copy Link

constant

constant
app/main.py Outdated
@ -0,0 +185,4 @@
raise_for_status(token_by_email_resp)
if token_by_email_resp.status_code != 200:
response = requests.post(
PeterSurda commented 2024-02-25 11:05:21 +00:00
Owner
Copy Link

Again extracting. Something like

if token_by_email_resp.status_code != 200:
   add_login()
else
   update_login()
Again extracting. Something like ``` if token_by_email_resp.status_code != 200: add_login() else update_login() ```
shailaja added 1 commit 2024-03-01 06:31:55 +00:00
shailaja force-pushed Services/part3 from 3bc031606e to 7d5390289e 2024-03-05 11:59:59 +00:00 Compare
This pull request can be merged automatically.
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b Services/part3 main
git pull origin Services/part3

Step 2:

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff Services/part3
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
swapnil
PeterSurda
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: PeterSurda/inoreader2readwise#4
No description provided.
Delete Branch "Services/part3"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?