Skip to content

WIP: Nvallinoto master for ws - #249

Open
jpahullo wants to merge 13 commits into
wip-ws-tasksfrom
nvallinoto-master
Open

WIP: Nvallinoto master for ws#249
jpahullo wants to merge 13 commits into
wip-ws-tasksfrom
nvallinoto-master

Conversation

@jpahullo

Copy link
Copy Markdown
Owner

No description provided.

@nvallinoto

nvallinoto commented Oct 25, 2023

Copy link
Copy Markdown

Hi Jordi, I'm able to restart again to work on this PR. I would like to take into consideration the 36 failing checks, whose details are not more visible. Which are the next steps ? I imagine some test classes should be included?

@jpahullo

jpahullo commented Nov 29, 2023

Copy link
Copy Markdown
Owner Author

Hi Jordi, I'm able to restart again to work on this PR. I would like to take into consideration the 36 failing checks, whose details are not more visible. Which are the next steps ? I imagine some test classes should be included?

Hi @nvallinoto ,

As you told me in private message, you can go on with this job. It seems good to me to solve this 36 issues. I was searching to rerun the workflow, but not luck (even though official documentation says this option exists). What comes into my mind is simply push again the same branch or making a new simple commit (like solving some spacing problem or so), and then committing again will rerun the workflow and you will be able to search for them and solve.

Regarding pending jobs, we have a very long conversation on #218. But I cannot figure out some of the last TODOs (if any) to recall the current state of the development. This would help to determine the pending tasks.

But, in general, as you say:

  1. Add testing:
    1. On the migration of data.
    2. On the login behind web services.
    3. Ensuring all prior tests still pass.
    4. Testing web services as a whole, step by step, alongside the whole merge request lifecycle: queued, ....
  2. Ensure existing webs for this plugin works as expected:
    1. In particular, the logs page should be redefined (its renderer) to cope with the new logs format and the new logs table.
    2. The settings page (just in case we missed some string on lang/)
  3. Adapt current web of addition new merging users to use the new queue. So that all is processed by the cron. Some adjustment may be convenient to just inform the merge request id and linking to it, to let the manager know where she can find the results.
    1. For this case, when admin adds a new request via web, it would be interesting to use the recently new field mergedbyuserid, to notify this user when the merge request was processed (with succes or error). This may be a case for a new event observer to deal with completion events (with success or error, no matter), that sends a notification email whenever the mergedbyuserid is filled in. No email should be send when some CLI tool or web service is used, since we have no idea of which user to notify to.
  4. Adapt CLI merger to queue merge requests, instead of executing them.
  5. Add a recommendation on the README that this task should be limited to have just one instance running in parallel (this setting is something supported by Moodle and can be configured on the config.php file). In my institution, for instance, it will be mandatory, to keep merging operation in-order, as they have to be performed.
    1. As an advanced option, it could be investigated if this can be done programatically in some way, and could be set on the settings.php, for instance.

And, in general, I think it would be all.

I attach this comment into the issue too.

Thanks for the work @nvallinoto.

Jordi

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants