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

N°5619 - Hide newsroom menu when no provider #363

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

Conversation

dflaven
Copy link
Contributor

@dflaven dflaven commented Nov 15, 2022

The newsroom menu can be hidden by configuration, but is not automatically hidden when there is no newsroom provider installed.
This pull request just hides the menu automatically in such a case.

@Molkobain Molkobain self-assigned this Nov 17, 2022
@Molkobain Molkobain added bug Something isn't working internal Work made by Combodo labels Nov 17, 2022
@Molkobain Molkobain added this to First review needed in Combodo PR dashboard via automation Nov 17, 2022
@dflaven
Copy link
Contributor Author

dflaven commented Nov 17, 2022

The second commit fixes a crash when there is actually no provider at all (not reproducible on a Community iTop, since there is the iTop Hub provider...)

@Molkobain Molkobain changed the title N°5619 - hide newsroom menu when no provider N°5619 - Hide newsroom menu when no provider Nov 17, 2022
@Molkobain Molkobain moved this from First review needed to Pending technical review in Combodo PR dashboard Nov 17, 2022
@@ -269,7 +270,7 @@ public function GetSubBlocks(): array
*/
public function IsNewsroomEnabled(): bool
{
return MetaModel::GetConfig()->Get('newsroom_enabled');
return (MetaModel::GetConfig()->Get('newsroom_enabled') && NewsroomMenuFactory::HasProviders());
Copy link
Member

Choose a reason for hiding this comment

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

But it does work on Essential/Professional products though, are you sure this is the root cause?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to #363 (comment) it's "crashing". Don't know what exactly he means with that.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the second commit it was crashing (during the construction of the iTop WebPage) on Essential/Professional because the (empty) newsroom menu object was not created (because there is not newroom provider on such products).

Copy link
Member

Choose a reason for hiding this comment

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

Discussed with Denis, I could not reproduce the issue on a Professional package. We would like to be able to reproduce the issue in order to ensure the fix is necessary and correct. Denis will dig further to have a reproduction procedure.

@Molkobain Molkobain moved this from Pending technical review to Pending contributor update in Combodo PR dashboard Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working internal Work made by Combodo
Projects
Status: Pending contributor update
Combodo PR dashboard
  
Pending contributor update
3 participants