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.3] [4.1 ScheduledTasks]Edits for run task #36725

Open
wants to merge 29 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

niharikamahajan02
Copy link
Contributor

Pull Request for Issue #36453 .

Summary of Changes

Edits for run task.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.1-dev labels Jan 18, 2022
@niharikamahajan02
Copy link
Contributor Author

Please review. Thanks

@tecpromotion
Copy link
Contributor

Please review. Thanks

Please check whether you have taken over all comments from your previous PRs.
Furthermore, I have the impression, but still have to test it, that again not all occurrences have been renamed.
Please also check this with a full text search over the whole repo.
And please keep the code up to date.

@niharikamahajan02
Copy link
Contributor Author

But sir , it is working fine after running the command (npm run build:js )
image

@@ -102,6 +102,14 @@ COM_SCHEDULER_OPTION_ORPHANED_SHOW="Show Orphaned"
COM_SCHEDULER_PERMISSION_TESTRUN="Test task"
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
COM_SCHEDULER_PERMISSION_TESTRUN="Test task"
COM_SCHEDULER_PERMISSION_RUN_TASK="Run task"

@niharikamahajan02 please check this constant and the string. You have already renamed it in the access.xml.

@tecpromotion
Copy link
Contributor

tecpromotion commented Jan 22, 2022

Should this also be adapted?

$mapping['onAjaxRunSchedulerTest'] = 'runTestCron';

and

public function runTestCron(Event $event)

Perhaps with runTaskCron

@tecpromotion
Copy link
Contributor

And maybe also replace onAjaxRunSchedulerTest with onAjaxRunSchedulerTask?

@ditsuke what do you think about this improvements?

@@ -19,12 +19,12 @@ const initRunner = () => {
const paths = Joomla.getOptions('system.paths');
const token = Joomla.getOptions('com_scheduler.test-task.token');
const uri = `${paths ? `${paths.base}/index.php` : window.location.pathname}?option=com_ajax&format=json&plugin=RunSchedulerTest&group=system&id=%d${token ? `&${token}=1` : ''}`;
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
const uri = `${paths ? `${paths.base}/index.php` : window.location.pathname}?option=com_ajax&format=json&plugin=RunSchedulerTest&group=system&id=%d${token ? `&${token}=1` : ''}`;
const uri = `${paths ? `${paths.base}/index.php` : window.location.pathname}?option=com_ajax&format=json&plugin=RunSchedulerTask&group=system&id=%d${token ? `&${token}=1` : ''}`;

@@ -89,7 +89,7 @@ public static function getSubscribedEvents(): array
$mapping['onContentPrepareForm'] = 'enhanceSchedulerConfig';
$mapping['onExtensionBeforeSave'] = 'generateWebcronKey';

$mapping['onAjaxRunSchedulerTest'] = 'runTestCron';
$mapping['onAjaxRunSchedulerTask'] = 'runTaskCron';
Copy link
Contributor

Choose a reason for hiding this comment

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

onAjaxRunSchedulerTask must also be fixed in the js files.
please check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh yes okk

@tecpromotion
Copy link
Contributor

Please check this two code comments.

* This method is responsible for the "test run" functionality in the Scheduler administrator backend interface.

* already running. This is a fair assumption if this test run was triggered through the administrator backend,

@tecpromotion
Copy link
Contributor

Also, please check the occurrences of 'test-task'.
This concerns file names, function calls and declarations.

@niharikamahajan02 sorry for all the comments but it's best to clean it straight away.

@niharikamahajan02
Copy link
Contributor Author

Also, please check the occurrences of 'test-task'. This concerns file names, function calls and declarations.

@niharikamahajan02 sorry for all the comments but it's best to clean it straight away.

No problem sir 😄 and yes it's better to clean them straight away now

@niharikamahajan02
Copy link
Contributor Author

Yes ,I would rename the file names, function calls and declarations also 👍

@niharikamahajan02
Copy link
Contributor Author

Please suggest if something needs to be updated now . Thanks

@ditsuke
Copy link
Contributor

ditsuke commented Jan 23, 2022

@tecpromotion

@ditsuke what do you think about this improvements?

Thanks for tagging! I really like the changes so far.

@niharikamahajan02
Copy link
Contributor Author

Yes sir , changed the file name now.

@niharikamahajan02
Copy link
Contributor Author

please review it once , I think there are some changes to be done

@richard67
Copy link
Member

please review it once , I think there are some changes to be done

@niharikamahajan02 It needs to resolve merge conflicts in files build/media_source/com_scheduler/joomla.asset.json and plugins/system/schedulerunner/schedulerunner.php. Maybe just use the clean files from the 5.1-dev branch and apply your changes again on them.

@niharikamahajan02
Copy link
Contributor Author

but they are already having "run test" in 5.1-dev

@richard67
Copy link
Member

but they are already having "run test" in 5.1-dev

@niharikamahajan02 What do you mean? Drone tests are failing, and if you go to the bottom of this PR on GitHub you clearly see there are the 2 mentioned files which have merge conflicts. If you don't know how to solve them we can help, but maybe you can try it yourself first like I have described.

hans2103 added a commit to hans2103/joomla-cms that referenced this pull request Jan 3, 2024
@HLeithner HLeithner changed the base branch from 5.1-dev to 5.2-dev April 24, 2024 09:09
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title [4.1 ScheduledTasks]Edits for run task [5.2] [4.1 ScheduledTasks]Edits for run task Apr 24, 2024
@brbrbr
Copy link

brbrbr commented Jul 7, 2024

I was pointed to this PR after a comment of mine on #43665 (comment)

  1. looks like the Commits on Dec 22, 2023 reverted all the changes, I see 'TEST_RUN' all over the place and no 'RUN TASK.
  2. breaks the site(UserFactoryAwareTrait not found), seems that some of the merges of branches failed. (tried gh pr checkout 36725 and git fetch upstream pull/36725/head:run-task).
  3. there are some merge issues with [5.2] Show a warning when there are due tasks #43491

This branch, based on the current 5.2-dev, should work: https://github.com/brbrbr/joomla-cms/tree/test2run-task

@bascherz
Copy link

I have tested this item 🔴 unsuccessfully on 6880884

Could not apply the patch, so could not test. This was done on a fresh 5.2.0-beta1 site.

ERROR MESSAGE:
The file marked for modification does not exist: plugins/system/schedulerunner/schedulerunner.php


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

@richard67
Copy link
Member

I have tested this item 🔴 unsuccessfully on 6880884Could not apply the patch, so could not test. This was done on a fresh 5.2.0-beta1 site.

ERROR MESSAGE: The file marked for modification does not exist: plugins/system/schedulerunner/schedulerunner.php
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36725.

@bascherz If you go to this PR on GitHub and scroll to the bottom you will see that this PR has conflicting files, so I cannot really be tested.

@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:54
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.3-dev.

@HLeithner HLeithner changed the title [5.2] [4.1 ScheduledTasks]Edits for run task [5.3] [4.1 ScheduledTasks]Edits for run task Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conflicting Files Feature Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.3-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.