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

Fix error handling issues #387

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

lalithr95
Copy link
Member

When ever an invalid tag is added rails error page will be displayed in dev mode. This PR displays error message to user through flash messages.

Alert view has been changed to support error datatypes compatibility.

@jywarren review please 😃

@jywarren
Copy link
Member

jywarren commented Mar 4, 2016

Hi, Lalithr95 - thank you, however the PR #381 is about to be merged. Your changes may have to be rebased and modified to play on top of that. Very grateful for your fixes, however, and I hope you do!

I announced #381 on the plots-dev list, so watch out there for similar planned merges which may require rebasing in the future.

@jywarren
Copy link
Member

jywarren commented Mar 4, 2016

Hello - thanks; I merged in #388 just now, and am ready to merge in your change. If you can rebase your changes on top of that and run tests, I'll test and merge this in.

By the way, I am super pleased that you submitted a test for this as well. As you can see our test suite is minimal and we are interested in building it out.

Thanks again!

@lalithr95
Copy link
Member Author

@jywarren #381 is a massive PR. After I rebased with master couple of tests are failing and they are not related to this PR.
NotesControllerTest#test_show_note and NotesControllerTest#test_don't_show_note_by_spam_author fail due to invalid method Author. Do you experience the same tests ?

I believe in TDD approach 😃

@jywarren
Copy link
Member

jywarren commented Mar 5, 2016

Yes, I added those tests, and I think it's highlighted the need for some
tighter management of models and some unit testing.

Is the version here the one you're testing? I can take a look.

It was a massive PR, but the bootstrap integration required some other code
changes... But perhaps I could've broken it up more.

Thanks a lot!
On Mar 4, 2016 9:58 PM, "Lalith Rallabhandi" notifications@github.com
wrote:

@jywarren https://github.com/jywarren #381
#381 is a massive PR. After I
rebased with master couple of tests are failing and they are not related to
this PR.
NotesControllerTest#test_show_note and
NotesControllerTest#test_don't_show_note_by_spam_author fail due to
invalid method Author. Do you experience the same tests ?

I believe in TDD approach [image: 😃]


Reply to this email directly or view it on GitHub
#387 (comment).

@lalithr95
Copy link
Member Author

Yea I'm using the same version. (master) Also I rebased with master

@jywarren
Copy link
Member

jywarren commented Mar 5, 2016

But I mean does this PR now include a rebased version so I can try to see
the test errors you're seeing?

Thanks!

On Fri, Mar 4, 2016 at 10:18 PM, Lalith Rallabhandi <
notifications@github.com> wrote:

Yea I'm using the same version. (master) Also I rebased with master


Reply to this email directly or view it on GitHub
#387 (comment).

@lalithr95
Copy link
Member Author

I rebased with master. You can test errors.

@jywarren
Copy link
Member

jywarren commented Mar 7, 2016

OK, I've pulled a copy of your PR and am running tests this morning. Thanks for your patience over the weekend.

@jywarren
Copy link
Member

jywarren commented Mar 7, 2016

Hmm, all tests passed for me. Where were you seeing problems? I think this is ready to merge.

@lalithr95
Copy link
Member Author

When I run rake test I get this output.
https://gist.github.com/lalithr95/de778629a1d31c5946f0

Do I have to setup any thing after your PR merge other day

@jywarren
Copy link
Member

jywarren commented Mar 7, 2016

That's odd, I definitely don't get those, and they were part of an issue I
fixed in my PR (I recognize them, but they were fixed later).

You're sure you pulled in the most recent publiclab/master before rebasing?
It seems like it if I can pull in your changes to my local copy of master
with fast-forward only.

Can you post the output of 'git log'?
On Mar 7, 2016 9:42 AM, "Lalith Rallabhandi" notifications@github.com
wrote:

When I run rake test I get this output.
https://gist.github.com/lalithr95/de778629a1d31c5946f0

Do I have to setup any thing after your PR merge other day


Reply to this email directly or view it on GitHub
#387 (comment).

@jywarren
Copy link
Member

jywarren commented Mar 7, 2016

Git log should match this:
https://github.com/publiclab/plots2/commits/master
On Mar 7, 2016 9:52 AM, "Jeffrey Warren" jeff@unterbahn.com wrote:

That's odd, I definitely don't get those, and they were part of an issue I
fixed in my PR (I recognize them, but they were fixed later).

You're sure you pulled in the most recent publiclab/master before
rebasing? It seems like it if I can pull in your changes to my local copy
of master with fast-forward only.

Can you post the output of 'git log'?
On Mar 7, 2016 9:42 AM, "Lalith Rallabhandi" notifications@github.com
wrote:

When I run rake test I get this output.
https://gist.github.com/lalithr95/de778629a1d31c5946f0

Do I have to setup any thing after your PR merge other day


Reply to this email directly or view it on GitHub
#387 (comment).

@lalithr95
Copy link
Member Author

Here is the output of git log which matches with master commits
https://gist.github.com/lalithr95/51539852082eab58f422

If tests are working fine you can actually merge it. May be problem is due to some setup.

@jywarren
Copy link
Member

jywarren commented Mar 7, 2016

Would you mind double checking by git cloning into a new copy of the
repository and running tests there?

I will merge, however. Thanks for the changes!
On Mar 7, 2016 9:58 AM, "Lalith Rallabhandi" notifications@github.com
wrote:

Here is the output of git log which matches with master commits
https://gist.github.com/lalithr95/51539852082eab58f422

If tests are working fine you can actually merge it. May be problem is due
to some setup.


Reply to this email directly or view it on GitHub
#387 (comment).

jywarren added a commit that referenced this pull request Mar 7, 2016
@jywarren jywarren merged commit ec038f6 into publiclab:master Mar 7, 2016
@lalithr95
Copy link
Member Author

cloning worked, I get all tests passed!

@jywarren
Copy link
Member

jywarren commented Mar 7, 2016

Great.
On Mar 7, 2016 10:12 AM, "Lalith Rallabhandi" notifications@github.com
wrote:

cloning worked, I get all tests passed!


Reply to this email directly or view it on GitHub
#387 (comment).

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.

None yet

2 participants