Problem/Motivation

working on #3100110: Convert update_calculate_project_update_status() into a class

I realized the logic in update_calculate_project_update_status() dealing with project_status from the update xml is not covered in tests.

Proposed resolution

Determine which values for project_status in update_calculate_project_update_status() are actually values that could in the Update XML. Test all those values

Remaining tasks

Review https://git.drupalcode.org/project/drupal/-/merge_requests/2329

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#29 3254426-nr-bot.txt2.38 KBneeds-review-queue-bot
#23 interdiff_18_23.txt19.88 KBsandeepsingh199
#23 3254426-23.patch10.62 KBsandeepsingh199
#18 interdiff_17-18.txt19.78 KBpooja saraah
#18 3254426-18.patch10.87 KBpooja saraah
#17 3254426-17.patch10.54 KBravi.shankar

Issue fork drupal-3254426

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#3100110: Convert update_calculate_project_update_status() into a class

Pushed up a commit to prove no tests fail

tedbow opened merge request !1545

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs work

Setting to needs work. Removing this logic and the tests still passing proves it is untested

kunal.sachdev made their first commit to this issue’s fork.

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

Needs work based on merge request comments

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

Needs work for comment in merge request. Note there is follow up that needs to be created there.

ravi.shankar made their first commit to this issue’s fork.

ravi.shankar’s picture

ravi.shankar commented

Here I have tried to resolve the comments on MR, please review.

Still needs work for creating follow up.

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

Very close! I just see 2 more points

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review

Made changes as per comment #13, please review.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

@ravi.shankar thanks, changes look good! @kunal.sachdev also thanks for work on this. RTBC 🎉

quietone’s picture

quietone commented
Version: 9.4.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Changing version to 10.0.x where this needs to be applied first. It will need a reroll, adding tag.

Does this still need tests? If not, please update the tags.

ravi.shankar’s picture

ravi.shankar commented
StatusFileSize
new 3254426-17.patch10.54 KB

Added patch for Drupal 10.0.x.

pooja saraah’s picture

pooja saraah commented
StatusFileSize
new 3254426-18.patch10.87 KB
new interdiff_17-18.txt19.78 KB

fixed failed commands against #17
Attached interdiff

rpayanm made their first commit to this issue’s fork.

rpayanm opened merge request !2329

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
immaculatexavier’s picture

immaculatexavier commented
Status: Needs review » Needs work

The patch #18 does not apply, so changing the status to Need Work.

sandeepsingh199’s picture

sandeepsingh199 commented
StatusFileSize
new 3254426-23.patch10.62 KB
new interdiff_18_23.txt19.88 KB

Yahh @immaculatexavier #18 patch is not working for me as well. Actually some files are missing on that patch, So I fixed those issue with help of #20 MR for 10.0.x and 10.1.x and upload the patch same.

sandeepsingh199’s picture

sandeepsingh199 commented
Status: Needs work » Needs review
Manoj Raj.R’s picture

Manoj Raj.R commented

Hi @sandeepsingh199
is the patch same as #20?. Looks pretty same to me.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs reroll +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

I do please #23 is the same as the MR.

Hiding all the patches as the MR seems the path forward for this.

Can the MR please be updated for 10.1

Since this is purely dealing with tests not sure it needs the needs tests tag.

quietone’s picture

quietone commented
Version: 10.0.x-dev » 11.x-dev
Issue summary: View changes
Status: Needs work » Needs review

Ignore the changes to the MR.

The patch in #23 is a correct reroll so let's just continue with that. I have started tests on 11.x

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems #23 is all green for 11.x so should be ready.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3254426-nr-bot.txt2.38 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs work » Needs review

Rebased MR2329 onto 11.x

quietone’s picture

quietone commented
Title: Most logic around project_status from update XML is not tested » Add tests for logic on project_status from update XML

smustgrave changed the visibility of the branch 3254426-untest-project-status to hidden.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Applied a small suggestion but rebase seems good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Fixing issue credit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 467483f to 11.x and 1cfb8f67f6 to 10.3.x and 5fcff92502 to 10.2.x. Thanks!

Backported to 10.2.x to keep tests aligned.

  • alexpott committed 5fcff925 on 10.2.x 
    Issue #3254426 by kunal.sachdev, quietone, ravi.shankar, tedbow,...

  • alexpott committed 1cfb8f67 on 10.3.x 
    Issue #3254426 by kunal.sachdev, quietone, ravi.shankar, tedbow,...

  • alexpott committed 467483fe on 11.x 
    Issue #3254426 by kunal.sachdev, quietone, ravi.shankar, tedbow,...

  • alexpott committed e2acd9e0 on 10.2.x 
    Revert "Issue #3254426 by kunal.sachdev, quietone, ravi.shankar, tedbow...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.2.x-dev » 10.3.x-dev

This broken 10.2.x because the messages were slightly different. Reverted from there.

quietone closed merge request !2329

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.