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

[5.2] remove trashed status from new article page #45065

Open
wants to merge 7 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

reem-atalah
Copy link

Pull Request for Issue #44962.

Summary of Changes

Remove the "Trashed" option from the "Status" drop-down list in the New article form.

Testing Instructions

Log in to Joomla! administrator
Toogle Menu -> Content -> Articles
Click on the "New" button
Click on the "Status" Drop down list
You will not see "Trashed" option

Actual result BEFORE applying this Pull Request

The trashed option appears in the status on the new article page
image

Expected result AFTER applying this Pull Request

The trashed option doesn't appear in the status on the new article page
image

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@reem-atalah reem-atalah changed the title 44962: remove trashed status from new article page [5.2] 44962: remove trashed status from new article page Mar 4, 2025
@brianteeman
Copy link
Contributor

Can you still change an article to be trashed?

@charvimehradu
Copy link
Contributor

charvimehradu commented Mar 4, 2025

hi,
the problem with this would be when we actually trash an article, the status would be shown as published (the default) and there would be no option to change the status of the article from the screen of the article. same as @brianteeman asked.

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 69c322d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45065.

@reem-atalah
Copy link
Author

Can you still change an article to be trashed?

yes
Screenshot from 2025-03-04 18-17-23
Screenshot from 2025-03-04 18-17-37
Screenshot from 2025-03-04 18-17-58

@reem-atalah
Copy link
Author

hi, the problem with this would be when we actually trash an article, the status would be shown as published (the default). same as @brianteeman asked.

Yes you're right thanks for highlighting this, I'll see it again

@reem-atalah
Copy link
Author

I have tested this item 🔴 unsuccessfully on 69c322d

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45065.

Can you give me more information

@brianteeman
Copy link
Contributor

@reem-atalah i marked it as unsuccessful as the issue you are fixing is not to offer trashed as an option when creating a NEW article but what you have done is to remove the ability to mark ANY article as trashed.

@fgsw
Copy link

fgsw commented Mar 4, 2025

I have tested this item 🔴 unsuccessfully on 69c322d

With Without
Untitled-pr Untitled

Issue #44962 was about new/unsaved articles. Now in saved articles is no Trashedpossible.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45065.

@reem-atalah
Copy link
Author

reem-atalah commented Mar 4, 2025

Yes I'll handle this now
Appreciated your testing and feedback

@reem-atalah
Copy link
Author

@brianteeman @fgsw Hi, I have edited the code and tested it to ensure that all article pages except the new article page have the "Trashed" option.
I appreciate your testing again, thanks in advance.

@brianteeman
Copy link
Contributor

This is looking more promising.

We do not add scripts inline like this. You should make it a standalone script and then it can also be used in all similar forms not just the admin content form.

I am not the best person to guide you on this but if you read https://manual.joomla.org/docs/general-concepts/javascript/adding-javascript/ it should point you in the right direction

@reem-atalah
Copy link
Author

Ok I'll see it, thanks for directing me

@reem-atalah
Copy link
Author

I can see that the scripts are set in one place which is Joomla-cms/media/com_content/js (in the case of the articles)
But the Media folder is not part of the Joomla-cms repo, it's a library, I have searched in the composer.json, and found "Joomla/MediaWiki", when I opened this repo I didn't find the Media folders, as seen in the Joomla-cms, so it's not the correct place. It's also not mentioned in the link you gave me before. Any clue to find the right place to update in the Media folder to add my js file?

@joomdonation
Copy link
Contributor

You can see source code of our JS in media_source folder. However, what you are doing here in this PR is not the right approach.

For adjusting a form, we should modify code of getForm method to make any adjustment. That would require understand our Form API

For this PR, below are how I would work on:

  • We need to remove an option from a ListField, so I first add a method to ListField class to allow removing an option base on the value. The method should be added after addOption method
  • Then modify code in getForm method of ArticleModel which I mentioned earlier, call $form->getField('state') to get the state field and then call the removeOption method which is added in earlier step to remove the option (in case $id = 0, for new article).

@fgsw
Copy link

fgsw commented Mar 5, 2025

@reem-atalah Can you change the title (without the issue-number) because the title is used in the changelog.

@reem-atalah reem-atalah changed the title [5.2] 44962: remove trashed status from new article page [5.2] remove trashed status from new article page Mar 5, 2025
@reem-atalah
Copy link
Author

@joomdonation Thanks a lot for directing me, I'll consider it. Appreciated

@reem-atalah
Copy link
Author

@joomdonation @brianteeman @fgsw I updated the PR, I hope it's now as expected, and be able to be merged

*
* @return ListField For chaining.
*
* @since 3.7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the current release. But we have some magic here so you dont have to keep changing the value if the merge is delayed etc

Suggested change
* @since 3.7.0
* @since __DEPLOY_VERSION__

@@ -231,6 +231,27 @@ public function addOption($text, $attributes = [])
return $this;
}

/**
* Method to remove the trashed option from the list field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Method to remove the trashed option from the list field.
* Method to remove the trashed option from new items in the list field.

@brianteeman
Copy link
Contributor

If you set the article option to use workflow and then try to create or edit an article you get the error

0 Call to undefined method Joomla\CMS\Form\Field\SpacerField::removeTrashedOption()

image

@reem-atalah
Copy link
Author

I can see that SpacerField, ListField, etc. (if there's another) are inherited from FormField, and I think this is the centralized place. Now it works, if there is another case that I can't see please inform me

@joomdonation
Copy link
Contributor

@reem-atalah Not all FormField has option, so it does not make sense to add that method to FormField class. Please follow instructions below:

  1. Add this method to ListField class as I mentioned before
/**
 * Method to remove an option from list field.
 *
 * @param   string  $value  The value of the option to remove
 *
 * @return  static  For chaining.
 *
 * @since   __DEPLOY_VERSION__
 */
public function removeOption(string $value): static
{
	foreach ($this->element->option as $option)
	{
		if ((string) $option['value'] === $value)
		{
			$dom = dom_import_simplexml($option);
			$dom->parentNode->removeChild($dom);
		}
	}

	return $this;
}
  1. Then in getForm method of ArticleModel, add the code below:
// Remove trashed option from state field for new article form
if ($id == 0)
{
	$field = $form->getField('state');

	if ($field !== false && $field->type === 'List')
	{
		$field->removeOption(-2);
	}
}

Hope you get the idea. Instead of add method to remove trashed option, the method would allow removing any options with the given value instead. And then in the model, you call $field->removeOption('-2') to remove the trashed option from the field

@reem-atalah
Copy link
Author

reem-atalah commented Mar 5, 2025

Ok, I thought this would be needed in all FormField, but yes I got how you want to proceed with the logic. Pretty to have the method more generic with an input, I can think of this after revising the code. Thanks for the clarification.

@reem-atalah
Copy link
Author

Updated, I hope now it fits our case

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on e9b313a

does what it says -= thanks

maintainers - should this be added to all other new forms (both site and admin)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45065.

@reem-atalah
Copy link
Author

Hello @brianteeman thanks for testing, Can I know what's next so my code can be merged?

@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on e9b313a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45065.

@joomdonation
Copy link
Contributor

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45065.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 6, 2025
@joomdonation
Copy link
Contributor

@reem-atalah The PR is now RTC. We will just need to wait for maintainer to review and merge it.

@reem-atalah
Copy link
Author

reem-atalah commented Mar 6, 2025

@joomdonation Great, thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-5.2-dev RTC This Pull Request is Ready To Commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants