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

Reply By Tweet Debugging #6093

Merged
merged 2 commits into from
Aug 15, 2019
Merged

Reply By Tweet Debugging #6093

merged 2 commits into from
Aug 15, 2019

Conversation

namangupta01
Copy link
Member

No description provided.

@namangupta01
Copy link
Member Author

@gauravano can we merge this?

@namangupta01 namangupta01 reopened this Aug 1, 2019
@namangupta01
Copy link
Member Author

Restarted travis.

@grvsachdeva
Copy link
Member

It's failing for all PRs @namangupta01

@namangupta01
Copy link
Member Author

Oh okay. So it's not related to mine. We can merge this then?

urls.each do |url|
puts ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"
puts "url.include? https:// -> #{url.include? "https://"}"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could use the start_with? string method instead of include? !

next unless url.include? "https://"

if url.last == "."
url = url[0...url.length - 1]
end

Copy link
Member

Choose a reason for hiding this comment

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

And here (lines 452 and 453) you could use the end_with? and chomp methods

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the review, @seabl!

@namangupta01
Copy link
Member Author

Hi @seabl , this is just for debugging. After we are done with the debugging we are going to remove it.
@jywarren can we merge this real quick so that we can further investigate what the actual problem is?
Thanks

@jywarren
Copy link
Member

jywarren commented Aug 3, 2019

Hi @namangupta01 i'm afraid I'd prefer to not merge a failing build, as it can mask other errors being merged. Can you try helping to solve #6094 do you think, as this is an urgent situation for all contributors?

Alternatively, is there really no way to test this happening in your dev environment or in unstable? Perhaps @SidharthBansal may be able to suggest a clever way to test it. Just to be clear, is it possible to get working locally in production mode, or in unstable? Thanks!

@namangupta01
Copy link
Member Author

I only have a couple of concern while testing on unstable which is unstable donot have upto date database to sync with the notes mention in tweets and if there are I don't know which one it is. And another concern is does login via twitter works on unstable.

@namangupta01
Copy link
Member Author

Sure I will have look at #6094 and try to solve it along with you guys.
Thanks

@jywarren
Copy link
Member

jywarren commented Aug 3, 2019 via email

@namangupta01
Copy link
Member Author

Hi, I tried this on local with a tweet from one of my account and replied to that tweet from another account and I was able to find the node from that tweet but in Production, I am not able to. I need a couple of information to get it fix and running it on production. First is what is the environment variable ENV["WEBSITE_HOST_PATTERN"] setted in there and second is what is the data we store in UserTag when we do login via twitter -- I need this just to confirm that everything will work fine on production because I am not able to run login via twitter on local and get that value. I guess these are the small things that we have to look into it. Afterwards, it will be good to run.
Also having discussion at #6085
Thanks

@jywarren
Copy link
Member

jywarren commented Aug 9, 2019 via email

@namangupta01
Copy link
Member Author

Thanks! Is this also same in stable.publiclab.org i.e WEBSITE_HOST_PATTERN=://publiclab.org/n/
Because I am downloading logs from http://stable.publiclab.org/cron_log1.log

@jywarren
Copy link
Member

jywarren commented Aug 9, 2019 via email

@namangupta01
Copy link
Member Author

It is weird. When I set the same on local I get the node id extracted from the link.

@namangupta01
Copy link
Member Author

Can we also merge this? Since I have tested on local and it's able to identify the node_id easily which it is not getting on production on checking logs? What do you think?

@jywarren
Copy link
Member

Yes! No danger that we'll fill up logs too fast or anything else to watch out for? Thanks!

@jywarren
Copy link
Member

I guess it's fine. But please open up an issue pointing to this PR so that we remember to take out these comments too! Thanks!

@jywarren jywarren merged commit 7b46cad into master Aug 15, 2019
@jywarren
Copy link
Member

OK, i hope to publish today but not sure. Thanks!

@jywarren
Copy link
Member

I'll leave a note in the chatroom if we manage to!

@namangupta01
Copy link
Member Author

Thanks!!! I will create a new issue for removing these comments so that we know we have to remove these comments.

@namangupta01
Copy link
Member Author

Hoping this could give us enough information to find out what is wrong exactly. Otherwise, we have to dig for more logs.

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.

4 participants