-
Notifications
You must be signed in to change notification settings - Fork 331
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
[ENG-4283]Feature/remove registration contributor settings #10333
base: develop
Are you sure you want to change the base?
[ENG-4283]Feature/remove registration contributor settings #10333
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes but we can discuss in person.
-
hmm ... the fix doesn't work as expected and it actually breaks OSF, please see the comments in diff for details.
-
In addition, have you checked your git history? The first commit in this PR is a merge commit, which shouldn't be there.
-
We need unit tests to cover this change. Unit tests are usually done within a ticket (when it is not part of a project team). Based on the fact that existing tests pass with the current change, I think we didn't have a specific test before and thus need new ones. One for testing the settings tab exists when it is a project and the other for non-existence when it is a registration. In both case, user needs to have proper permission.
-
Nitpicking: we could use a slightly better PR description, which includes some screenshots of before/after from local testing :) and adding a link to the ticket also helps
(Note: I have a few screenshots for code review but I am not able to upload them. I guess GitHub is doing some maintenance now. But I will add them early in the morning.) done
Hi Longze, thank you for the review but this is a draft PR. The change is not working but was pending environmental fixes. Thank you for the time confirming the Settings change with a functional environment. Let me use the thread to get mine up to speed so I can further test this. |
Before reviewing this PR, I asked you whether this PR is ready to review. You said yes and you changed the ticket to "In Review". That's when I assigned myself. As we discussed in Slack, I just learned that you were having problems setting up your local env, and thus you were not able to test your change. This explains why the fix doesn't work. I can help with your local env since it maybe a different issue from what I ran into if you need. |
c5f1b72
to
3ead321
Compare
Purpose
The purpose of these changes is to remove the 'Settings' tab from contributor registrations.
Changes
Logic has been updated to remove Settings from the Registration contributor page.
Screenshot(s)
QA Notes
Areas of risk?
There should be no area of risk, but sending out an informational email or notification that the tab has been removed from the project header and can be found on the main navbar would be helpful.
Documentation
Help guides may need to reflect the update.
Side Effects
There should be no side effects unless other routes invoke this one.
Ticket
https://openscience.atlassian.net/browse/ENG-4283