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

E2126. Refactor account_request_controller.rb #2117

Merged
merged 26 commits into from
Feb 4, 2022
Merged

E2126. Refactor account_request_controller.rb #2117

merged 26 commits into from
Feb 4, 2022

Conversation

majuabir
Copy link
Contributor

Team members
Abir Majumder
Connor Smith
Joshua Myers

Wiki with all the proposed changes:
https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2021_-_E2126._Refactor_account_request_controller.rb

ConnorS1110 and others added 15 commits October 18, 2021 12:55
@ConnorS1110
Run migrate
c12813d
@ConnorS1110
Refactored account_request_controller
0244a4b
@ConnorS1110
Fixed refactoring of create_approved_user method
eed880b
@Jamerack
Separated notifying of super users about new account request from cre…
28a306a 
…ation of record
@Jamerack
Renamed user_existed to user_exists. Moved requested_user validation …
d22a572 
…and saving to own method
@Jamerack
Made error message more readable. Changed how paramsare being passed …
93e7f64 
…to save_reqeuested_user
@ConnorS1110
Updated error message on line 26
71e62c9
@ConnorS1110
Merge branch 'beta' of https://github.com/majuabir/expertiza into beta
57a7f48
@ConnorS1110
Revert "Updated error message on line 26"
a06283a 
This reverts commit 71e62c9.
@ConnorS1110
Revert "Merge branch 'beta' of https://github.com/majuabir/expertiza …
5854747 
…into beta"

This reverts commit 57a7f48, reversing
changes made to 71e62c9.
@ConnorS1110
Reverted to working code
cc01a35
@ConnorS1110
Updated tests to include new error message
c25ad14
@Jamerack
Refactored create_requested_user_record into 3 methods and made varia…
2fed94c 
…bles/errormessages more readable
@majuabir
buggy captcha implementation; testing if works on server
3e535ef
@ConnorS1110
Updated error message on line 34
6d2243e
@expertiza-bot
Copy link
Contributor

1 Message
💬 Thanks for the pull request, and welcome! 🎉 The Expertiza team is excited to review your changes, and you should hear from us soon.

This repository is being automatically checked for code-quality issues using Code Climate.
You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a pull request is considered ready to review.

Also, please spend some time looking at the instructions at the top of your course project writeup.
If you have any questions, please send email to expertiza-support@lists.ncsu.edu.
UUID 2 Warnings
c951 ⚠️ You should commit changes to the DB schema (db/schema.rb) only if you have created new DB migrations.
Please double check your code. If you did not aim to change the DB, please revert the DB schema changes.
c51d ⚠️ You are modifying Gemfile or Gemfile.lock, please double check whether it is necessary.
You are suppose to add a new gem only if you have a very good reason. Try to use existing gems instead.
Please revert changes to Gemfile.lock made by the IDE.
UUID 2 Errors
65de 💥 You are including debug code in your pull request, please remove it.
b9a1 💥 You changed .gitignore; please double-check whether this is necessary.

Generated by expertiza-bot

@expertiza-bot
Copy link
Contributor

expertiza-bot commented Oct 21, 2021 •
edited

1 Message
💬 Thanks for the pull request, and welcome! 🎉 The Expertiza team is excited to review your changes, and you should hear from us soon.

This repository is being automatically checked for code-quality issues using Code Climate.
You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a pull request is considered ready to review.

Also, please spend some time looking at the instructions at the top of your course project writeup.
If you have any questions, please send email to expertiza-support@lists.ncsu.edu.
UUID 1 Warning
c51d ⚠️ You are modifying Gemfile or Gemfile.lock, please double check whether it is necessary.
You are suppose to add a new gem only if you have a very good reason. Try to use existing gems instead.
Please revert changes to Gemfile.lock made by the IDE.
c951 ✅ You should commit changes to the DB schema (db/schema.rb) only if you have created new DB migrations.
Please double check your code. If you did not aim to change the DB, please revert the DB schema changes.
UUID ✅ Nice work.
b9a1 ✅ You changed .gitignore; please double-check whether this is necessary.
65de ✅ You are including debug code in your pull request, please remove it.

Generated by expertiza-bot

@expertiza-travisci-bot
Copy link

expertiza-travisci-bot commented Oct 21, 2021 •
edited

Hey @majuabir,
Your changes look good to me! 🎉

@coveralls
Copy link

Coverage Status

Coverage decreased (-18.6%) to 38.176% when pulling 6511a9f on majuabir:beta into 6ad3607 on expertiza:beta.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-18.6%) to 38.176% when pulling 6511a9f on majuabir:beta into 6ad3607 on expertiza:beta.

@coveralls
Copy link

coveralls commented Oct 29, 2021 •
edited

Coverage Status

Coverage increased (+0.02%) to 45.268% when pulling 3036201 on majuabir:beta into e8c559f on expertiza:beta.

johnbumgardner
johnbumgardner requested changes Jan 30, 2022
View reviewed changes
Copy link
Collaborator

@johnbumgardner johnbumgardner left a comment

Choose a reason for hiding this comment

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

needs manual testing and code tests

app/controllers/account_request_controller.rb Outdated
def create_approved_user
# If a user isn't selected before approving or denying, they are given an error message
if params[:selection] == nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

use .nil?

@johnbumgardner johnbumgardner self-assigned this Feb 2, 2022
@johnbumgardner johnbumgardner merged commit 70d66a2 into expertiza:beta Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbumgardner johnbumgardner johnbumgardner requested changes

Assignees

@johnbumgardner johnbumgardner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@majuabir @expertiza-bot @expertiza-travisci-bot @coveralls @johnbumgardner @ConnorS1110 @Jamerack