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-5617] Phase 3: Check new develop changes #10618

Conversation

opaduchak
Copy link

@opaduchak opaduchak commented May 15, 2024

Purpose

Sync feature/python-upgrade with newest changes from develop.
Mostly everything is fine. From what We have seen, Uditi’s PR should not bring us conflicts. But John’s PR had several nuances for us and caused several conflicts, as function check_access was replaced by check_resource_permissions, several lines in tests were rewritten using nose (which we do not use now), and some functions were rewritten or split.
For now the conflicts were resolved.

Changes

  • Merged conflict fixes
  • fixes to tests which failed due to John's changes

QA Notes

Please make verification statements inspired by your code and what your code touches.

  • Verify
  • Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-5617

uditijmehta and others added 6 commits May 9, 2024 14:08
… Interface (CenterForOpenScience#10608)

## Purpose

Adding a boolean attribute to the AbstractProvider model. This change enables administrators to control whether a provider is displayed on the OSF Discover pages directly from the admin interface.

## Changes

- Added advertiseOnDiscoverPage boolean field to AbstractProvider
- Updated the admin forms and templates to include a checkbox for advertiseOnDiscoverPage
- Adjusted the PreprintProviderDisplay view to serialize the new advertiseOnDiscoverPage field

---------

Co-authored-by: Uditi Mehta <uditimehta@COSs-MacBook-Pro.local>
…ce#10584)

* refactor get_auth
* conditionally get credentials and settings from GravyValet

---------

Co-authored-by: John Tordoff <>
…ity (CenterForOpenScience#10613)

* fix errors retrieving BaseFileNode and file path for metrics

---------

Co-authored-by: John Tordoff <>
Co-authored-by: Jon Walz <jon@cos.io>
* Don't try to get fileversions for folders
---------

Co-authored-by: Jon Walz <walzj@COSs-MBP.lan>
Co-authored-by: John Tordoff <Johnetordoff@users.noreply.github.com>
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.

Looks great and 💯 for handling the merge conflicts. A few requests before I am back on Tuesday, thanks!

  • Fix trivial import style in addons/base/views.py
  • Update PR title (ticket number, no longer WIP, etc.)
  • Add a little bit details to the PR description, this doesn't have to be long, what @ly-mariia mentioned in Slack to me is good enough
  • Can any of you verify locally that "after this PR is merged in to the feature branch, there is no conflicts merging the feature branch back to the develop"?

On question, related to the last item above, I think this 35d5512 is the actual merging develop into feature branch commit that includes conflict fixing. If so, (a note for myself) I will rename it to Merge develop into feature/python-upgrade and fix conflicts.

Another note for myself: back-up the feature branch before merge, create a dedicated phase 3 branch for deployment after merge.

addons/base/views.py Outdated Show resolved Hide resolved
@opaduchak opaduchak changed the title [WIP]Feature/develop updates [ENG-5617] Phase 3: Check new develop changes May 20, 2024
@opaduchak
Copy link
Author

opaduchak commented May 20, 2024

On question, related to the last item above, I think this 35d5512 is the actual merging develop into feature branch commit that includes conflict fixing. If so, (a note for myself) I will rename it to Merge develop into feature/python-upgrade and fix conflicts.

Indeed, it is.
And regarding local check, I doubt that this is necessary, because this commit already resolved all conflicts. (I compared develop and this branch on github, and there aren't any conflicts)

@cslzchen
Copy link
Contributor

Cc @felliott, due to conflicts fix (35d5512) when merging develop into the feature branch, rebase, merge --no-ff and pushing back couldn't work. Now merging this PR in GitHub directly.

@cslzchen cslzchen merged commit baba6ef into CenterForOpenScience:feature/python-upgrade May 21, 2024
6 checks passed
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

5 participants