Overview

Follow-up to #3478299: Implement basic auto-save. We now need to a meet the auto-save requires of #3455753: Milestone 0.2.0: Early preview

In #3478299 we implemented the basics, we auto-save the entity and allow the editor to load those changes back.

For Milestone 0.2 we will need be able to retrieve all auto-save entities and metadata about the auto-saves

List of all entities that have been changed

The API endpoint needs to have following information:

  1. Entity Label
  2. Entity type
  3. Entity ID
  4. Changed timestamp
  5. Author of the change

Proposed resolution

  1. Create an auto-save service that will replace NotTheGoodAutoSaveTrait

    This service will have to

    • Auto-save an individual entity, including metadata about the save
    • Retrieve the auto-save state of an entity
    • Provide a list of all auto-saved entities with there meta
  2. Replace the existing use of the NotTheGoodAutoSaveTrait in ApiPreviewController and ApiLayoutController with use of the new service

    Eventually the client may want to have have separate calls to the server to for auto-save operation but for now these are working and not changing that keeps the scope smaller and just to the backend

  3. Provide a controller that will use the service to retrieve a listing the entity auto-save and their metadata

Remaning tasks

  • Add conflict IDs and updated data to body of the 409
  • update openapi.yml to reflect possible 409 and presence of the data_hash key
  • Add image style
  • Rename controller and consider consolidation with the publish all one
  • Rebase off 0.x now that 408 is in

User interface changes

Issue fork experience_builder-3489743

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Couple of questions on the current progress:

* I don't think we should pass the data as an array, can we not just keep it as a string and save the performance overhead of serialize\unserialize being called (and particularly the memory impact). Would likely need our own schema for the tempstore to bypass the serializing of the data column
* Should we also be considering langcode here in the identifiers?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

I pushed up a start that just replaces the current trait with a service. It doesn't change the basic functionality.

It does not add any of the extra metadata, timestamp, author, etc to the auto-save yet.

One thing we will have to consider eventually is that by #3455753: Milestone 0.2.0: Early preview we will be dealing with new entities, not just existing ones. We will also have a "publish all" button that takes all of the auto-saved work and saves them to real entity revisions. For new entities that are created in XB that means we can not rely on entity type/entity id for the auto-save key because they will not exist as entities yet. Maybe \Drupal\experience_builder\AutoSaveManager::save should return back the auto-save key(a uuid?) so that client can request the auto-save state back later when it there is no entity involved.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

Also right now the e2e still don't test auto-save. There is hack in the test to get around it

we should probably commit #3481736: Adapt E2E tests to work with auto-save, this issue. But it doesn't block it now

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Create AutoSave service and HTTP API to retrieve all entities with pending changes » [PP-1] Create AutoSave service and HTTP API to retrieve all entities with pending changes
Related issues: +#3489994: Create an endpoint to publish all auto-saved entities

Rebased the MR on top of #3489994: Create an endpoint to publish all auto-saved entities

Adding PP-1 to the title to reflect that should go in first and then this should be rebased

larowlan opened merge request !432

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

https://git.drupalcode.org/issue/experience_builder-3489994/-/merge_requ... shows the changes on top of #3489994: Create an endpoint to publish all auto-saved entities

The merge request seen above is including that branch

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review

The code here is passing

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

Changes since my last review look good. I think this is basically ready. Will review again after #3489994: Create an endpoint to publish all auto-saved entities is and we can remove those changes

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: [PP-1] Create AutoSave service and HTTP API to retrieve all entities with pending changes » Create AutoSave service and HTTP API to retrieve all entities with pending changes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

This is ready for review now, there's one question on the MR about whether we should ship with an image style for the avatars or not

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

I talked to @effulgentsia about this issue and #3489994: Create an endpoint to publish all auto-saved entities which together will allow the UI to create "Publish all" button

I think we missed something between these 2 issues

Here is the UI proposed workflow

  1. User clicks "Ready to publish"
  2. User sees a list of all entities in the auto-save state that are ready to be published and the dates they were last changed
  3. User clicks "Confirms"
  4. if auto-save states matches what the user just saw the all the changes will be published. This means there can't be any new changes since the user saw the list. No new entities, no entities missing, no new changes to the entities
  5. if there are changes since the user saw the list they will be prompted to confirm after seeing the new list.

So as it is now ApiPublishAllController doesn't take any input from the client but we probably need to change that where the client sends back some info that it received from ApiAutoSaveController. Otherwise the user could see a list of changes to be published wait an hour while other changes are made, without them being aware, click "publish all" and then they have published things they are not away of.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Needs work

Ok started a solution to #13 will comment on the MR

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes

Added https://www.drupal.org/project/experience_builder/issues/3491459

And in doing so it makes me feel the best naming for the controller here would be something involving 'pending changes'

Remaining tasks:

* Add conflict IDs and updated data to body of the 409
* update openapi.yml to reflect possible 409 and presence of the data_hash key
* Add image style
* Rename controller and consider consolidation with the publish all one
* Rebase off 0.x now that 408 is in

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Status: Needs work » Needs review

This is ready for review now 🙌

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

I think this issue is done but there are couple problem in 0.x that makes test fail.

#3492734: Empty-canvas e2e test is failing randomly and has leftover wait() call that shouldn't be there causes e2e tests to fail.

#3492722: Update XB to require Drupal 11.2 found an error that should be fixed in the core issue #3492705: Install modules with container_rebuild_true set by themselves

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#3492740: ui/tests/support/commands.js should set 'hostname' = '/'

Created follow-up #3492740: ui/tests/support/commands.js should set 'hostname' = '/'

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: Unassigned » traviscarden

assigned to @traviscarden for openapi.yml approval/review

traviscarden made their first commit to this issue’s fork.

traviscarden’s picture

traviscarden commented
Assigned: traviscarden » tedbow
Status: Needs review » Reviewed & tested by the community

Just the usual little formatting niggle and it looks good, @tedbow.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@traviscarden thanks for the review !

Test are still failing because of #3492722: Update XB to require Drupal 11.2 and #3492734: Empty-canvas e2e test is failing randomly and has leftover wait() call that shouldn't be there(e2e)

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

Test are passing except for cspell which is know issue fixed in #3493299: Fix cspell errors we didn't affect that file

empty-canvas.cy.js is failing but that is known issue being worked on in #3492734: Empty-canvas e2e test is failing randomly and has leftover wait() call that shouldn't be there

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

  • tedbow committed 1ef6cb3d on 0.x authored by larowlan 
    Issue #3489743 by larowlan, tedbow, traviscarden, lauriii: Create...
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Fixed

Done! thanks @larowlan and everyone!

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: tedbow » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

FYI, follow-up fixed: #3505590: Follow-up for #3489743: add missing `enforced` dependency to xb_avatar image style, to ensure it is uninstalled together with XB.