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.

Sign up for GitHub

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

Jump to bottom

Update: Removed unused buttons. Log out function fails upon button click #198

Merged
merged 45 commits into from
Oct 6, 2023
Merged

Update: Removed unused buttons. Log out function fails upon button click #198

merged 45 commits into from
Oct 6, 2023

Conversation

ErinClaudio
Copy link
Collaborator

@ErinClaudio ErinClaudio commented Sep 26, 2023 •
edited
Loading

Describe your change in plain English.

  • Removed extraneous dropdown options, retaining only the "Log Out" button.
  • Updated the "Log Out" button to successfully log out the user.
  • Adjusted system tests to account for dynamic organization slugs and improved test reliability.

Link to the issue

#195

Concerns

I have reservations about the sad path test for the "non-authenticated user attempts to log out" scenario. While the test passes, I believe it can be enhanced. Would appreciate guidance or a review on this particular test.

@kasugaijin
Copy link
Collaborator

Could you check to see if your change is causing the one test in the pipeline to fail?
Thanks!

@edwinthinks
Copy link
Collaborator

Looks like the failed test has nothing to do with the changes.

LoginTest#test_: when logging in as a staff member should direct to the organization's adoptable pets page. :
  Ferrum::ProcessTimeoutError: Browser did not produce websocket url within 5 seconds, try to increase `:process_timeout`. See https://github.com/rubycdp/ferrum#customization

This is usually caused because the CI machine is too slow. We can fix this by making a tweak in our settings. @ErinClaudio @kasugaijin we can safetly just ignore that. I'll open a issue to resolve it.

@kasugaijin
Copy link
Collaborator

Looks like the failed test has nothing to do with the changes.

LoginTest#test_: when logging in as a staff member should direct to the organization's adoptable pets page. :
  Ferrum::ProcessTimeoutError: Browser did not produce websocket url within 5 seconds, try to increase `:process_timeout`. See https://github.com/rubycdp/ferrum#customization

This is usually caused because the CI machine is too slow. We can fix this by making a tweak in our settings. @ErinClaudio @kasugaijin we can safetly just ignore that. I'll open a issue to resolve it.

Ah yes. This is out flaky test. It failed a couple times ealier this evening too.

@ErinClaudio ErinClaudio force-pushed the Update-options-in-the-avatar-dropdown-menu-to-be-only-Sign-Out branch from 557ca60 to a4ed92d Compare September 29, 2023 15:59
@ErinClaudio
Copy link
Collaborator Author

ErinClaudio commented Sep 29, 2023 •
edited
Loading

@kasugaijin @edwinthinks Thank you for the update!

ErinClaudio and others added 12 commits September 29, 2023 13:14
@ErinClaudio
Update with debugging
3627eea
@ErinClaudio
Update singout works I still need to clean up messy code
ffa00f4
@jadekstewart3 @ErinClaudio
Pets index styling (#204)
5c7af82 
* added layout import to pets dashboard, removed javascript from head partial

* utilize geeks ui styling and move components

* Implement requested changes

* Change header variable to match others

* remove previous change that was causing funky formatting
@nsiwnf @ErinClaudio
Add closing tag
a13f253
@nsiwnf @ErinClaudio
Sticky nav bar (#217)
adf210e
@nsiwnf @ErinClaudio
Updating index page to match geekui styling
c548999
@nsiwnf @ErinClaudio
Update pet show screen to match geeksui mock
081f145
@nsiwnf @edwinthinks @ErinClaudio
Adding staff index page (#223)
f6a06ee 
Co-authored-by: Edwin Mak <[email protected]>
@marlena-b @ErinClaudio
Invert association between Location and AdopterProfile (#225)
acbc281
@heyapricot @ErinClaudio
Added the gretel gem to manage breadcrumbs. Added breadcrubms to dash…
7c655e1 
…board pets index and show
@yuricarvalhop @ErinClaudio
228 remove location columns on organizations table and add zipcode to…
11802f5 
… locations table (#230)

* Remove location fields to Organization

* Add zipcode field to locations

* Fix seeds

* Add zipcode to location factory

* WIP: Remove organization.city usage from views/organizations/home/index.html.erb

* Run automatic fixer

* Remove unecessary comment
@ErinClaudio
Update with debugging code removed
e9cef2d
@ErinClaudio ErinClaudio marked this pull request as ready for review October 3, 2023 22:30
edwinthinks
edwinthinks requested changes Oct 3, 2023
View reviewed changes
Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

Thanks @ErinClaudio for your PR. I left you a comment let me know if you have any Qs

app/views/layouts/dashboard.html.erb Outdated Show resolved Hide resolved
nsiwnf and others added 22 commits October 5, 2023 16:26
@nsiwnf @edwinthinks @ErinClaudio
Adding staff index page (#223)
3809657 
Co-authored-by: Edwin Mak <[email protected]>
@marlena-b @ErinClaudio
Invert association between Location and AdopterProfile (#225)
71f54ea
@heyapricot @ErinClaudio
Added the gretel gem to manage breadcrumbs. Added breadcrubms to dash…
16db26a 
…board pets index and show
@yuricarvalhop @ErinClaudio
228 remove location columns on organizations table and add zipcode to…
be92edb 
… locations table (#230)

* Remove location fields to Organization

* Add zipcode field to locations

* Fix seeds

* Add zipcode to location factory

* WIP: Remove organization.city usage from views/organizations/home/index.html.erb

* Run automatic fixer

* Remove unecessary comment
@ErinClaudio
Added happy path test
6e93546
@ErinClaudio
Update with pseudocodefor tests
97040a2
@ErinClaudio
Merge branch 'main' into Update-options-in-the-avatar-dropdown-menu-t…
040ac6d 
…o-be-only-Sign-Out
@ErinClaudio
Update testing merge conflicts
c33f2a3
@ErinClaudio
Update with git merge main
e4addcf
@ErinClaudio
Update alta seed file fixed
e8d7319
@ErinClaudio
Update new line added
473bd84
@ErinClaudio
Update to schema
e4f4450
@ErinClaudio
Update to merging/rebase issues
a90990a
@ErinClaudio
Correct syntax for button_to with do end added
82b1992
@ErinClaudio
Syntax cleanup
f1f5e77
@ErinClaudio
Update with sad path test
64b433b
@ErinClaudio
Update with improved test
c0373a5
@ErinClaudio
Update with hardcoded page for testing
203f226
@ErinClaudio
Merge branch 'main' into Update-options-in-the-avatar-dropdown-menu-t…
c3bb666 
…o-be-only-Sign-Out
@ErinClaudio
Update with local passing tests that fail on CI
6b3af44
@ErinClaudio
Update with hard coded route
d7c8652
@ErinClaudio
Update org slug
1cc5fa8
edwinthinks
edwinthinks approved these changes Oct 6, 2023
View reviewed changes
Copy link
Collaborator

@edwinthinks edwinthinks left a comment

Choose a reason for hiding this comment

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

Thanks @ErinClaudio ! LGTM!

@edwinthinks edwinthinks merged commit 8f0d89b into main Oct 6, 2023
@edwinthinks edwinthinks deleted the Update-options-in-the-avatar-dropdown-menu-to-be-only-Sign-Out branch October 6, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwinthinks edwinthinks edwinthinks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ErinClaudio @kasugaijin @edwinthinks @jadekstewart3 @nsiwnf @marlena-b @heyapricot @yuricarvalhop @Ptrboro