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

Added errorMessage partial to user sessions form #8973

Closed
wants to merge 1 commit into from
Closed

Added errorMessage partial to user sessions form #8973

wants to merge 1 commit into from

Conversation

gucci-ninja
Copy link
Contributor

Fixes #8959 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@welcome
Copy link

welcome bot commented Jan 8, 2021

Thanks for opening this pull request! This space is protected by our Code of Conduct - and we're here to help.
Dangerbot will test out your code and reply in a bit with some pointers and requests.
Also please refer here for installation help 💿
There may be some errors, but don't worry! We'll work through them with you! 👍🎉😄
It would be great if you can tell us your Twitter handle so we can thank you properly?

@gitpod-io
Copy link

gitpod-io bot commented Jan 8, 2021

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@b3a9aec). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8973   +/-   ##
=======================================
  Coverage        ?   74.41%           
=======================================
  Files           ?      100           
  Lines           ?     6075           
  Branches        ?        0           
=======================================
  Hits            ?     4521           
  Misses          ?     1554           
  Partials        ?        0           

@noi5e
Copy link
Contributor

noi5e commented Jan 8, 2021

Hi @gucci-ninja! I took a look at the tests that were failing... Hmm... That's weird, they don't seem to be related to what you're working on.

Just FYI (and maybe you know this already), you can look at the test results here by clicking on Details:
Screen Shot 2021-01-07 at 6 22 15 PM
Then the stacktrace will give you the place to look in the codebase to do some more research (line 115 in this file):
Screen Shot 2021-01-07 at 6 23 18 PM

Sometimes the tests will fail not necessarily because of the PR. If they fail relatively quickly, like 5-10 minutes, you can run them again by clicking on Re-run jobs here:
Screen Shot 2021-01-07 at 6 26 30 PM

I just tried re-running them for you, let's see what happens!

@gucci-ninja
Copy link
Contributor Author

gucci-ninja commented Jan 8, 2021

@noi5e yeah I noticed so I haven't put the PR up for review yet. Lots of undefined method 'errors' for nil:NilClass errors. I also couldn't find a @user_session defined in the controller so that might be causing it.

@noi5e
Copy link
Contributor

noi5e commented Jan 8, 2021

Hmm, weird. I did notice that the continuous integration tests that were failing the second time were different, and there were new errors that weren't there the first time.

I think it would help reviewing your code if you could paste screenshots of the Rails stacktrace nil errors?

@@ -1,6 +1,7 @@
class UserSessionsController < ApplicationController
before_action :require_no_user, only: [:new]
def new
@user_session = UserSession.new
Copy link
Contributor

@noi5e noi5e Jan 8, 2021

Choose a reason for hiding this comment

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

It seems like this wasn't specified in the original issue?

Sorry I can't be more helpful, this isn't really my expertise, but perhaps @cesswairimu @jywarren or @sagarpreet-chadha could help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I added that in after noticing the tests were failing. Thanks a lot for looking into this btw. I'll post the stack trace as well

@codeclimate
Copy link

codeclimate bot commented Jan 8, 2021

Code Climate has analyzed commit 59f223e and detected 0 issues on this pull request.

View more on Code Climate.

@coder645
Copy link

coder645 commented Jan 8, 2021

@gucci-ninja it's not because of @user_session not being in the controller. I added that and tried but still gives errors. Ig this is a general problem and is caused because of another PR not being merged.
Could you close this PR for now? I'll ask to reopen the issue after this problem is resolved.
Cheers!

@gucci-ninja
Copy link
Contributor Author

@coder645 Sounds good! I'll keep an eye out for when it reopens! Thanks for your help everyone 💯

@gucci-ninja gucci-ninja closed this Jan 8, 2021
@cesswairimu
Copy link
Collaborator

Hi @gucci-ninja, there are these new ftos #8974 and #8975 if you are still looking for another fto. Thanks

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.

Add errorMessage partial in user_sessions/_form.html.erb
4 participants