Skip to content

N°4032 - On UserRequest, service subcategory no more limited by request_type #786

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.

Sign up for GitHub

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

Jump to bottom
Open
v-dumas wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

N°4032 - On UserRequest, service subcategory no more limited by request_type #786

v-dumas wants to merge 2 commits into from
+21 −13

Conversation

@v-dumas
Copy link
Contributor

@v-dumas v-dumas commented Dec 9, 2025

Base information

Question Answer
Related to a SourceForge thread / Another PR / Combodo ticket?
Type of change? Bug fix / Enhancement / Translations

Symptom (bug) / Objective (enhancement)

Reproduction procedure (bug)

  1. On iTop x.y.z
  2. With PHP x.y.z
  3. First go there
  4. Then do that
  5. ...
  6. Finally, see that...

Cause (bug)

Proposed solution (bug and enhancement)

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

  • ...
  • ...
  • ...

@CombodoApplicationsAccount CombodoApplicationsAccount added the internal Work made by Combodo label Dec 9, 2025
Hipska
Hipska reviewed Dec 9, 2025
View reviewed changes
Hipska
Hipska approved these changes Dec 11, 2025
View reviewed changes
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Shouldn't the request_type field now be dependant from subcategory_id and made read-only?

@v-dumas
Copy link
Contributor Author

v-dumas commented Dec 11, 2025

Shouldn't the request_type field now be dependant from subcategory_id and made read-only?

For the dependency, it would make sense if it was recomputed it in the DB_EVENT_COMPUTE_VALUES, but as it is done on a DB_EVENT_BEFORE_WRITE, the dependency would have no effect. At the end the result, is the same, so it's good enough, I will tackle other topics.
For the read-only, I would have transformed it in ExternalField. The reason why it is not read-only is that out of the box, in the console, Service subcategory isn't mandatory and user may set the request_type without setting a Service. At least that's what I was told by those who have the most experience in iTop Datamodel Design back in year 2010 ;-)

@Hipska
Copy link
Contributor

Hipska commented Dec 11, 2025

It would have been nicer indeed if it was implemented as DB_EVENT_COMPUTE_VALUES, the user could see the updated value when changing a service/sub.

Anyway, accepted as-is.

rquetiez
rquetiez reviewed Dec 16, 2025
View reviewed changes
datamodels/2.x/itop-request-mgmt/datamodel.itop-request-mgmt.xml
<filter><![CDATA[SELECT ServiceSubcategory WHERE service_id = :this->service_id AND (ISNULL(:this->request_type) OR request_type = :this->request_type) AND status != 'obsolete']]></filter>
<filter><![CDATA[SELECT ServiceSubcategory WHERE service_id = :this->service_id AND status != 'obsolete']]></filter>
<dependencies>
<attribute id="service_id"/>
Copy link
Contributor

@rquetiez rquetiez Dec 16, 2025

Choose a reason for hiding this comment

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

Why keep this second explicit dependence (it will be computed anyhow)?

Copy link
Contributor

@Hipska Hipska Dec 16, 2025

Choose a reason for hiding this comment

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

To not be able to select ServiceSubcategory from another Service. What do you mean with "it will be computed"?

Copy link
Contributor Author

@v-dumas v-dumas Dec 16, 2025

Choose a reason for hiding this comment

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

Hi Hipska, since about 2 or 3 years, the field dependencies are useless when due to a filter. iTop automatically add the dependency on field specified in the filter. But dependency remains useful when due to a PHP computation.

Copy link
Contributor

@Hipska Hipska Dec 16, 2025

Choose a reason for hiding this comment

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

So you're saying that in the console selecting a Service, and not having this dependency at subcategory, the refresh of that list will happen anyways because :this->service_id is used in the filter?

Copy link
Contributor Author

@v-dumas v-dumas Dec 16, 2025

Choose a reason for hiding this comment

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

Yes, console and portal

datamodels/2.x/itop-request-mgmt/datamodel.itop-request-mgmt.xml
<code><![CDATA[ public function ComputeValues()
{
// Compute the priority of the ticket
$this->Set('priority', $this->ComputePriority());
Copy link
Contributor

@rquetiez rquetiez Dec 16, 2025

Choose a reason for hiding this comment

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

As soon as this changes, I would like it to be fully aligned with the recommendation => event based.
Two handlers could be called : EvtComputePriority and EvtComputeRequestType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rquetiez rquetiez rquetiez left review comments

+1 more reviewer

@Hipska Hipska Hipska approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

internal Work made by Combodo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@v-dumas @Hipska @rquetiez @CombodoApplicationsAccount