Skip to content

Fix(planning): Avoid double AJAX call to get_events on FullCalendar initiazation #19462

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

Open
wants to merge 9 commits into
base: 10.0/bugfixes
Choose a base branch
from

Conversation

stonebuzz
Copy link
Contributor

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes !37230

During the plugin initialization, we observe two AJAX calls made to load events.

image

The first one results from the calendar initialization,

image

while the second is triggered by the computation and loading of the last default view.

image

trigger by this

        // Load the last known view only if it is valid (else load default view)
        const view = this.calendar.isValidViewType(options.default_view) ?
            options.default_view :
            default_options.default_view;
        this.calendar.changeView(view);

This second call is redundant, as the default view has already been computed server-side (in PHP) and does not need to be reloaded / check on the client side.

            $options = [
                'full_view'    => true,
                'default_view' => $_SESSION['glpi_plannings']['lastview'] ?? 'timeGridWeek',  // HERE
                'license_key'  => $scheduler_key,
                'resources'    => self::getTimelineResources(),
                'now'          => date("Y-m-d H:i:s"),
                'can_create'   => PlanningExternalEvent::canCreate(),
                'can_delete'   => PlanningExternalEvent::canPurge(),
            ];

Screenshots (if appropriate):

@stonebuzz stonebuzz changed the title Fix(planning): Avoid double AJAX call to get_events on FullC initiazation Fix(planning): Avoid double AJAX call to get_events on FullCalendar initiazation Apr 15, 2025
@stonebuzz stonebuzz self-assigned this Apr 15, 2025
@stonebuzz stonebuzz added the bug label Apr 15, 2025
@stonebuzz stonebuzz added this to the 10.0.19 milestone Apr 15, 2025
Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

See #16439, the lines you removed prevent a fatal error when the current view is invalid (e.g. a view from a plugin that was since disabled).

@AdrienClairembault
Copy link
Contributor

If you want to improve this, maybe the options could be checked on the PHP side.

@stonebuzz
Copy link
Contributor Author

If you want to improve this, maybe the options could be checked on the PHP side.
Voici une version améliorée, corrigée, complétée, rendue professionnelle et traduite en anglais de ton message :

A verification on the PHP side is not possible because the resourceTimeline plugin is always loaded by FullCalendar. It is then up to FullCalendar, on the client side, to check whether the corresponding library is actually available and whether a valid license_key has been provided (see the PHP code in the AdvancedDashboard plugin):

$PLUGIN_HOOKS['planning_scheduler_key']['advancedplanning'] = function () {
    return 'GPL-My-Project-Is-Open-Source';
};

GLPI (on the PHP side) has no knowledge of which JavaScript plugins are effectively loaded. Furthermore, plugin names often differ from the view types used by the user. For instance, the timeGridWeek view is provided by the timeGrid plugin.
I have therefore refactored the view check logic to fall back to the default view only when the currently requested view is Invalid.

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

Did you try it with the plugin to make sure the issue doesn't happen ?

Because in #16439 defaultView is not set in the constructor so we have no choice but to call this.calendar.changeView(view).

I noticed that it was added back in #16788, which probably bring back the original issue again...

@stonebuzz
Copy link
Contributor Author

Indeed !!

I did a small refactor.

I removed the use of defaultView from constructor, which caused the following error when the Advanced Planning plugin is disabled and the resourceWeek view type is no longer available:
👉 #16439

The idea is to handle the view only after initialization.

If the requested view is valid, it will be applied. Otherwise, it will fall back to the default view.

@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Apr 18, 2025

It seems to me that with the changes, If options.default_view is valid, it is never applied.

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

See last comment.

@stonebuzz
Copy link
Contributor Author

last commit should solve the problems

@stonebuzz
Copy link
Contributor Author

stonebuzz commented Apr 22, 2025

The latest commit also removes code that caused the get_events AJAX endpoint to be called twice.

// Save the original datesRender function, which is later overwritten by the "selectable" option
const originalDatesRender = GLPIPlanning.calendar.getOption('datesRender');
GLPIPlanning.calendar.setOption('selectable', false);
window.setTimeout(function() {
    GLPIPlanning.calendar.setOption('selectable', true);
    originalDatesRender(info); // Restore datesRender
}, 500);

This code was only intended to temporarily disable and then re-enable date selection controls.
Calling originalDatesRender ensured that navigation buttons were properly redrawn.

However, the current implementation (since #19184) triggered a duplicate call to get_events when switching views:

View change -> get_events -> render -> datesRender -> refresh -> get_events

Since events are added via a modal, the date selection controls aren't accessible at that point.
Therefore, there's no need to disable them temporarily, which justifies the removal of this code.

@cedric-anne cedric-anne requested a review from Rom1-B April 22, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants