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°3441 - Portal: Fix failure to open an object containing a link to an archived object #523

Merged
merged 5 commits into from
Aug 10, 2023

Conversation

larhip
Copy link
Contributor

@larhip larhip commented Jul 11, 2023

Portal : cannot open an object containing a link to an archived object

@Molkobain Molkobain self-assigned this Jul 11, 2023
@Molkobain
Copy link
Contributor

Moving to functional review as it will be this afternoon and we might have to discuss expected behavior.

@Molkobain
Copy link
Contributor

Molkobain commented Jul 11, 2023

Functional review:

  • Ok to avoid crashes and display the information in read-only instead
  • We would go furthermore with the display and be similar to what is done in the back office with the "archive" icon prefixed (see screenshot below)

image

Thanks for the PR Lars!

@Molkobain Molkobain added the bug Something isn't working label Jul 11, 2023
@Molkobain Molkobain self-requested a review July 11, 2023 21:05
@larhip
Copy link
Contributor Author

larhip commented Jul 12, 2023

Thanks @Molkobain for your feedback.

Display an archive icon in the portal as well is done now.

It might be checked during technical review, wehster there is some smarter way to do so.
I was also thinking about using the method DBObject::GetHyperlink to do it in a similar way like the back office. So something like this:

if ($this->oField->GetCurrentValue() !== null && $this->oField->GetCurrentValue() !== 0 && $this->oField->GetCurrentValue() !== '')
{
	// Note : AllowAllData set to true here instead of checking scope's flag because we are displaying a value that has been set and validated
	$oFieldValue = MetaModel::GetObjectWithArchive($sFieldValueClass, $this->oField->GetCurrentValue(), true, true);
	$sFieldHtmlValue = $oFieldValue->GetHyperlink();
}
else
{
	$sFieldHtmlValue = Dict::S('UI:UndefinedObject');
}

But in this case the archived icon will be displayed as well and the attribute data-toggle="itop-portal-modal" is missing..

@Molkobain
Copy link
Contributor

Indeed, methods in \DBObject or \AttributeDefinition are backoffice-oriented, so we can't rely on them for now. What you did goes in the right direction, I'll take a closer look during the review :)

@Molkobain Molkobain changed the title N°3441 - Portal : link to an archived object N°3441 - Portal: Fix failure to open an object containing a link to an archived object Jul 12, 2023
@Molkobain Molkobain added this to the 3.1.1 milestone Aug 8, 2023
@Molkobain
Copy link
Contributor

Accepted during functional for 3.1.1 as it can be blocking for people archiving objects.

@Molkobain Molkobain changed the base branch from develop to support/3.1 August 9, 2023 09:41
@Molkobain
Copy link
Contributor

Hello Lars,

Could you rebase your branch on support/3.1 so we can merge it?
If you are not comfortable doing so, feel free to ask. :)

@larhip
Copy link
Contributor Author

larhip commented Aug 9, 2023

Hi Guillaume,

should be done now, could you please check again?

@Molkobain
Copy link
Contributor

I'm sorry Lars but I still can't apply suggestions :/

Could you apply the 2 I just made, I didn't noticed them earlier but you changed the usages to the deprecated API.
You should be able to apply the suggestion directly from GitHub GUI.

Then I'll be able to merge, the rebase was done correctly 👍

@larhip
Copy link
Contributor Author

larhip commented Aug 10, 2023

Hi Guillaume,

I have just committed your suggestions, so you should be able to merge now.
BTW: When I got it correctly it is not able to set the option "editable by maintainers" if the fork belong to an organization.
Which means it should be possible to set the option for an PR is created from larhip/iTop but not from itomig-de/iTop.
Maybe I could do a quick test today :-)

@Hipska
Copy link
Contributor

Hipska commented Aug 10, 2023

BTW: When I got it correctly it is not able to set the option "editable by maintainers" if the fork belong to an organization. Which means it should be possible to set the option for an PR is created from larhip/iTop but not from itomig-de/iTop.

Ooh, this is interesting.. The same problem I'm running into.

@Molkobain
Copy link
Contributor

BTW: When I got it correctly it is not able to set the option "editable by maintainers" if the fork belong to an organization. Which means it should be possible to set the option for an PR is created from larhip/iTop but not from itomig-de/iTop.

Ooh, this is interesting.. The same problem I'm running into.

Indeed, it's a shame though 😁

@Molkobain
Copy link
Contributor

Hi Guillaume,

I have just committed your suggestions, so you should be able to merge now. [...]

Perfect thanks, merge incoming!

@Molkobain Molkobain merged commit caf3076 into Combodo:support/3.1 Aug 10, 2023
@larhip
Copy link
Contributor Author

larhip commented Aug 10, 2023

Just tested it, it is for real related to the owner of the fork (organization vs. personal):

Personal fork:
image

Organization fork:
image

@Molkobain
Copy link
Contributor

Thanks for the feedback Lars! @piRGoif you might interested by this.

@Hipska
Copy link
Contributor

Hipska commented Aug 10, 2023

hmm, could be worked around that?

@piRGoif
Copy link
Contributor

piRGoif commented Aug 10, 2023

Wooo ! Many thanks Larhip ! I didn't saw this in the documentation :(
I already saw issues opened for GitHub bugs/enhancement, maybe we could open one ?

@Hipska
Copy link
Contributor

Hipska commented Aug 10, 2023

Please do and let us know the reference..

@piRGoif
Copy link
Contributor

piRGoif commented Sep 5, 2023

Well, the issue already exists : https://github.com/orgs/community/discussions/5634
I've just added a comment. Feel free to add yours, and vote !

piRGoif pushed a commit that referenced this pull request Sep 5, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants