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

[ENG-4283]Feature/remove registration contributor settings #10333

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

chth0n1x
Copy link
Contributor

@chth0n1x chth0n1x commented Feb 21, 2023

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)

Pasted Graphic 3

QA Notes

  • Verify the Settings tab is removed in all project views
  • Verify that user Settings can still be accessed via the navbar

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

@chth0n1x chth0n1x changed the title [ENG-4283]Feature/remove registration contributor settings [DRAFT][ENG-4283]Feature/remove registration contributor settings Feb 22, 2023
@cslzchen cslzchen self-requested a review March 1, 2023 20:06
Copy link
Contributor

@cslzchen cslzchen left a 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

website/templates/project/project_header.mako Outdated Show resolved Hide resolved
@chth0n1x
Copy link
Contributor Author

chth0n1x commented Mar 3, 2023

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.

@cslzchen
Copy link
Contributor

cslzchen commented Mar 3, 2023

but this is a draft PR

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.

@chth0n1x chth0n1x force-pushed the feature/remove-registration-contributor-settings branch from c5f1b72 to 3ead321 Compare June 1, 2023 14:45
@chth0n1x chth0n1x changed the title [DRAFT][ENG-4283]Feature/remove registration contributor settings [ENG-4283]Feature/remove registration contributor settings Jun 5, 2023
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