Skip to content
This repository has been archived by the owner on Mar 7, 2019. It is now read-only.

Member Group Picker No Longer Works with Archetype #261

Closed
Nicholas-Westby opened this issue Apr 1, 2015 · 20 comments
Closed

Member Group Picker No Longer Works with Archetype #261

Nicholas-Westby opened this issue Apr 1, 2015 · 20 comments

Comments

@Nicholas-Westby
Copy link
Contributor

I built a site with Umbraco 7.1.8 that included an Archetype that has a property of type Member Group Picker. I upgraded to Umbraco 7.2.2 a while back and only now noticed that I can no longer picker member groups (well, I can, but the changes are lost when I save/publish the node).

To confirm this wasn't some peculiarity about my install, I tried with a fresh install of Umbraco 7.2.2 and Archetype 1.7.2. Here's an animation that shows the selected member group disappear when I click publish:
groups

Note that the other member group picker that is outside of the Archetype works fine.

Here is my Archetype config model:
config-model

@kgiszewski
Copy link
Owner

I'm getting 404 on both of your images at the moment.

So before getting in and looking at it, Archetype doesn't control anything internal to any data types so if this is happening it's probably due to something changing with the data type itself.

I think you logged a similar issue with the color picker having issues as well (since it was updated).

Wonder if this is the trend and a bit concerned.

@Nicholas-Westby
Copy link
Contributor Author

Strange, I see the images fine (even in different browsers and operating systems). Maybe GitHub uses a CDN that takes some time to propagate or something.

Yeah, I think it was definitely a change to the Umbraco core that caused the issue, though it seems to only apply when used in combination with Archetype.

My guess is the relevant changes are these two commits, made shortly after the release of Umbraco 7.1.8:

@kgiszewski
Copy link
Owner

Yea not even avatars are loading at the moment. I'll have to look tomorrow if I get a chance.

Re: commits...

I'll have to see if I can add some logging in my local version to see what's going on (provided I can replicate your issue).

@Nicholas-Westby
Copy link
Contributor Author

Cool. I'm doing some troubleshooting too. Here's another relevant commit (more recent than the other two):

@Nicholas-Westby
Copy link
Contributor Author

The first clue I've seen is that the "formSubmitting" handler in memberGroupPicker() gets hit after Archetype's "formSubmitting" handler. I expect the intention is that Archetype's handler would be called last. Not quite sure if that is causing this issue, but it could be important anyway, as it seems Archetype is doing some validation of the values, which at that point may not yet be set (in the case of the member group picker, the model value gets set in the formSubmitting handler). Here's the code I'm talking about:

@Nicholas-Westby
Copy link
Contributor Author

I made a very temporary workaround to get this working on a production environment that needs it. I'll leave it to you to investigate the root cause and maybe address it with the Umbraco core team. Basically, I noticed the propertyValueChanged function in Archetype was getting called after both formSubmitting event handlers (I am guessing after the form got submitted to the server). My workaround was to set value in the member group picker when an item is added rather than just when the form submits. Here's the code I changed:
moved-code

Here's the Umbraco source code that relates to: https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web.UI.Client/src/views/propertyeditors/membergrouppicker/membergrouppicker.controller.js#L60

Note that there are problems with this workaround (e.g., it doesn't account for removed items). However, since it's hopefully a very temporary workaround, I'm not going to worry too much about it (I'm hoping you or the Umbraco core team can get a fix out).

@Nicholas-Westby
Copy link
Contributor Author

Seems like this other issue is related: #105

@Shazwazza
Copy link

I've made a couple comments: umbraco/Umbraco-CMS@e4a8edd?w=1#commitcomment-10535243

I'm not sure how archetype works but if it works in the core, then Archetype would need to be able to make it work with itself. As mentioned in the comments, i can make the change in the core for this property editor but that's not a 'fix' because any property editor is more than allowed to set it's model based on another value during formSubmitting.

@kgiszewski
Copy link
Owner

I'll see what I can do in Archetype to try to catch these types of updates.

@kgiszewski
Copy link
Owner

@Shazwazza I understand the "tail doesn't wag the dog" so I totally understand that the core shouldn't be concerned with how Archetype works. Thanks for the insight btw 👍

@Shazwazza
Copy link

It's just that there is probably other property editors (maybe even in the core currently) that update it's model during this event, so it just needs to take that into account.

@kgiszewski
Copy link
Owner

@Nicholas-Westby The quickest solid fix for you might be to create a Legacy Member Picker by cloning the old code and creating a custom property editor.

@kgiszewski
Copy link
Owner

@Shazwazza Yep will do, just not sure if/how it's possible :)

@Nicholas-Westby I'll hit this tomorrow.

@Nicholas-Westby
Copy link
Contributor Author

Cool. By the way, it seems like this scenario should be made to be possible by the Umbraco core. That is, it should be possible for a package like Archetype to be able to intercept values in property editors before the form submits.

Maybe that can be done by watching for changes and immediately responding to them (i.e., before the form submits). That may be tricky, as I would guess any event handlers would be fired after the current event handler (namely, for the "formSubmitting" event) runs. And I assume the next thing to happen would be that the browser would send the request to the server (after which the event to respond to value changes would fire), which would obviously be a problem. If, however, the change event handlers are immediately called (on the change rather than after the current even handler finishes), that could be a workable approach.

One potential alternative would be to attach a priority to "formSubmitting" handlers so that higher priority ones occur after lower priority ones. Not sure if there is an elegant way of handling that, but if there is I'm guessing it'd require an Umbraco core change.

Anyway, just some ideas. Will leave it to you guys to figure something awesome out :-)

@kgiszewski
Copy link
Owner

Ok, I hope I can explain what I've found and tried.

What Archetype does now...

To update values, Archetype implements a watcher on each property under it's control and if a value changes, it updates the parent model.

https://github.com/imulus/Archetype/blob/master/app/directives/archetypeproperty.js#L210-L215

We're using $on("formSubmitting") for validation only.

The problem...

As-is, Archetype doesn't save the value of the member picker because the watcher never fires until it is too late (form has submitted). It is consistent with what @Nicholas-Westby has said.

We are using the formSubmitting event only for the purposes of validation at the moment which leads me to believe we might have some validation issues with properties that update their model during the same event (this is a side problem).

If we have Archetype try to update during formSubmitting, a race condition or ordering issue occurs. i.e. the value of the child properties aren't set when it tries to update it's main model.

Things I've tried...

  • I've tried to listen to specific events like modal-hide but that event returns empty selection list.
  • I've tried to see how validation comes into play but the best I can do is to stop the form from submitting, update the values, and then try to resubmit the form; but that got really ugly fast.
  • I've tried using formSubmitted but by this point updating the Archetype is pointless.

What does work...

  • Setting the model outside of the final event in the chain formSubmitting. But as @Shazwazza said, this isn't prohibited to do.
  • Rebuild a modified version of the property type that doesn't update during the formSubmitting event.
    This leads to double work and an eco-system of alternate versions of core data types.

What might work...

  • Adding an event that is meant for things like Archetype, a last chance update (i.e. $on("noREALLYtheFormIsSubmitting.")). But this is probably bad form.
  • Adding a queue or ordering to the formSubmitting. From what I understand, this is bad form as well for events. Events aren't supposed to be able to fire in any order.
  • Create core events that can be captured: i.e. $on('treePickerPickedSomething), $on('pickerListRemovedItem') and return the payload each time. Overkill maybe? This will lead to bad data as some of the properties do data pruning during the formSubmitting event instead of when the modal closes, etc.

Scratching my head...

  • Trying to wrap my head around why a model should be updating only on formSubmitting. i.e. How does validation see the proper value?

This is probably a conversation for a larger group, but I'm not sure which direction to take this.

@Nicholas-Westby
Copy link
Contributor Author

A coworker and I have come up with a promising solution, but it will require both an Umbraco core change and an Archetype change. The issue really stems from this line of code: https://github.com/umbraco/Umbraco-CMS/blob/ded1def8e2e7ea1a4fd0f849cc7a3f1f97cd8242/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.js#L53

Umbraco is using $broadcast to notify scopes of the "formSubmitting" events. $broadcast sends event notifications starting with ancestor scopes and working download to descendant scopes. This means that Archetype will always process the "formSubmitting" event before the property editors it contains. However, Angular does provide an $emit function, which sends events out in the reverse order (i.e., from the descendant scope and upward to the ancestor scopes).

So, we'd have to modify any property editors that listen for the "formSubmitting" event so that they emit another event after they are done. This event might be called "formSubmittingProcessed". In particular, we could add a line after this one to $emit that event: https://github.com/umbraco/Umbraco-CMS/blob/ded1def8e2e7ea1a4fd0f849cc7a3f1f97cd8242/src/Umbraco.Web.UI.Client/src/views/propertyeditors/membergrouppicker/membergrouppicker.controller.js#L72

That would look something like this:
example

Then, in Archetype, you'd listen for that event and update Archetype's model based on the model of the property editor that initiated the event.

@kjac
Copy link
Contributor

kjac commented Apr 3, 2015

Hey all. I've been experimenting with this for some time now, and I think I
have a solution that does not require any core changes and that will work
with any data type.

Unfortunately I've got to take care of some family business for a few days,
but I'll push a PR as soon as I'm back home.
On Apr 3, 2015 3:00 AM, "Nicholas-Westby" notifications@github.com wrote:

A coworker and I have come up with a promising solution, but it will
require both an Umbraco core change and an Archetype change. The issue
really stems from this line of code:
https://github.com/umbraco/Umbraco-CMS/blob/ded1def8e2e7ea1a4fd0f849cc7a3f1f97cd8242/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.js#L53

Umbraco is using $broadcast to notify scopes of the "formSubmitting"
events. $broadcast sends event notifications starting with ancestor scopes
and working download to descendant scopes. This means that Archetype will
always process the "formSubmitting" event before the property editors it
contains. However, Angular does provide an $emit function, which sends
events out in the reverse order (i.e., from the descendant scope and upward
to the ancestor scopes).

So, we'd have to modify any property editors that listen for the
"formSubmitting" event so that they emit another event after they are done.
This event might be called "formSubmittingProcessed". In particular, we
could add a line after this one to $emit that event:
https://github.com/umbraco/Umbraco-CMS/blob/ded1def8e2e7ea1a4fd0f849cc7a3f1f97cd8242/src/Umbraco.Web.UI.Client/src/views/propertyeditors/membergrouppicker/membergrouppicker.controller.js#L72

That would look something like this:
[image: example]
https://cloud.githubusercontent.com/assets/5933222/6976350/66351708-d95d-11e4-88d6-ff3d69aa59db.png

Then, in Archetype, you'd listen for that event and update Archetype's
model based on the model of the property editor that initiated the event.


Reply to this email directly or view it on GitHub
#261 (comment).

@kgiszewski
Copy link
Owner

@kjac 👍

@Shazwazza
Copy link

@Nicholas-Westby not a huge fan of the broadcast + re-emit. This all needs to be possible without core changes and suddenly everyone needs to manually emit an event just to make Archetype work which is not what we want.

Potentially archetype could re-broadcast the formSubmitting event from it's own scope which would force all child property editors to receive the event before archetype processes things... but that comes with its own consequences in that there might be a circumstance where a property editor is only ever expecting a single event broadcast during a save event and in some edge cases this might cause an issue with a property editor.

@Nicholas-Westby
Copy link
Contributor Author

@Shazwazza Thanks for the tips, though @kjac and @kgiszewski already have a solution that requires no core changes. I think the basic idea is that they place a directive directly after the property editor and have that broadcast a new event ("archetypeFormSubmitting") on the "formSubmitting" event. Since it appears after the "formSubmitting" event, the "archetypeFormSubmitting" event occurs later. I didn't look too closely at their code, but I think that's the general idea.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants