Improve and separate mapping UI

Oh hadn't considered the many to many possibility. I love this even more than when we discussed it in DC! :)

We discussed mapping to the feed and to the feed items. Would this be implemented in two of these areas shown in the wireframe? Maybe we could do to sub tabs off of the mapping tab to seperate the management of the two target objects?

working on it

The per-node mapping with the new style of mapping just works.
But the code needs work because:
- UI needs tiny improvements at some places (in-line form elements, etc)
- per-content-type mapping needs to be tested

I'll let you solidify this before doing a deeper review. Here is my initial feedback:

http://skitch.com/alexbarth/bmkf8/development-seed-technological-solutio...

It would be great to have this in the next beta release. Nice work so far.

Oh, I had a patch going to but was waiting for #397682: Let mappers declare mapping targets. Will try to review and post it next week.

Ok, here's a swing at it. Stole some things from Aron's patch and did some other things a lot different. Still needs a lot of work but its very functional.

I applied patch, it doesn't seem to list existing mappings. Does it?

I just applied the patch fresh on another site and saw what the problem was. It will require a cache refresh for them to show up because they are put into a table in a theme function that won't be in the registry.

Attached is a patch that better handles the delete links with a confirmation form.

Actually, this is probably ready we can start reviewing it for problems. I don't think there are really any glaring issues keeping it from being committed.

Small update to the previous patch to filter out previously mapped items from the select options.

Fixed:

- Redirection on node type form
- Not 'Feed' but 'Feed item' on mapping form.
- Minor code style fixes

My questions and observations:

1)
I don't understand why we introduce feedapi_mapper_add_mapping() and feedapi_mapper_delete_mapping(). Couldn't we completely replace an existing mapping with every form submission? I'm not quite sure what the additional layer to _feedapi_mapper_store_mapping() brings us.

2)
Delete confirm form needs a better legible mapping string: http://www.skitch.com/alexbarth/bqa88/are-you-sure-you-want-to-delete-th...

3)
Found an issue with standard mapping sources that aren't available on a feed node: #458240: Standard sources should be always available on feed node.

4)
Minor: Let's move the new theme_feedapi_mapper_form function right under the other theme function in feedapi_mapper.module. I know that function ordering in feedapi_mapper.module needs some love in general, but let's worry about that in another patch (adding a UI like this may justify adding an admin.inc and a theme.inc.)

I am really excited about this improvement, thanks for your work so far. It's an important piece in making this module grow up...

Hi, will these improvements be committed? I see needs work, but questions are observations, does this mean the patch needs work?
greetings, Martijn

I worked a little bit on the patch.
I found some bugs in #11:
you can submit the form without any warnings when "select a source" is selected, this is nonsense, i added a validation hook.
When I added a mapping with a target of a Date field for example (grouped under a parent target!), the index was invalid here:
$form['#field_map'][$node_path], (around line 120).
When you added some mappings, deleted all of them, tried to add mapping again, SQL error appeared

I fixed all the above issues.

I fixed all of these and also:
1) I don't understand why we introduce... [additional layer] . Hm. It's not an additional layer. It's a replacement and I have to admit, i like it :) _feedapi_mapper_store_mapping() is removed.
2) Fixed, now the message appears nice.
3) Not fixed yet.
4) Reordered, added comment.

It still needs work, for example because of #11 question 3).

#11 question 3) is solved as well, a new patch is attached.
I realized that feedapi_mapper in general emits some php notices, but it's not really related to this patch. One related ticket: #486102: Too complicated descriptions array, php notice, try to use non-existing variable

#14: Aron, could you remove all fixes (code style, notices) from this patch that are not related to the task at hand? This would make it much simpler to read the patch - thank you!

I'm not yet really happy about the description and the example feed item in a collapsible fieldset. The description should actually go into hook_help - but that will make the header section really large and push the actuall UI under the fold. The example feed item could stay in a collapsible for now - I can't even think of a meaningful alternative here for now. Need to think more about this.

Otherwise this is starting to look really solid.

Other reviewers welcome.

At first shot, I removed all fixes from the patch what weren't related strictly. (as requested in #15) There were not many at all, one isset() and one bad identation at hook_menu() and one array initialization, . I did not included the solution for #486102: Too complicated descriptions array, php notice, try to use non-existing variable of course, in #13, it was only a notice.

I found one serious problem, consider the following situation:
The content-type has a mapping and i define a mapping on per-node basis. When I delete all the mappings on per-node, i just suddenly get the per-content-type mapping, so i cannot really have an empty mapping on a per-node basis if per-content-type mapping is defined. I think this is not a neat behaviour. I'd propose a 'Revert' button, so first the content-type mapping is there, and after something is modified, the revert button shows up with some description. If everything is deleted at the node, it remains empty and with the Revert button. you can load the per-node-type mapping. What do you think?
I do not want to sink work to implement this after we have consensus on how it should work.

Found a bug in #16: only the standard elements appeared on per-node mapping page.
Fixed.

#17: That is a problem, right. I see it separate from the mapping UI though.

Hi, great movement.
Stupid thinking may be, but could there be some conditional working between the mapping?
So say for location_cck, the location_country field needs to be the 2-figures iso-code, say US for United States. But a lot of feeds just have something like "united states". Could may be something like, for this field, if the same as a database field A, use database field B for the mapping.
In this example the countryname should be the countrycode to have correct working of cck_location.
Hopefully correct place to ask, while I think with new Gui, it would be great to have this option, say through the rules module?
Thanks for considering this. And of course will I start a new issue if this is not the appropriate place for it.

EDIT: Made new issue for this: http://drupal.org/node/501730.

Looking forward for new Gui progress in .dev
Greetings,
Martijn

#20: this is OT. You're asking for mapping based on synonyms (comparable to taxonomy synonyms) - there may be an open issue for that. If synonyms are supported by the mapping targets (for example like on taxonomy), FEMP can theoretically support that.

In #15, "The description should actually go into hook_help" - alex_b
Correct me, if it overlook something, but hook_help() is generally for providing non-context-sensitive help. Do you see a clear way for example to access to the node object in hook_help()? Sure, it is possible to examine the url arguments and load it, but it's not so clean as feedapi_mapper_page gets the data from the menu system.
If #17 is not strictly related to this ticket, i think this is ready to review.
Let's do one step per ticket, if we find a nice way to show the description and the example item to the user, we can add it, it's independent from how the mapping can be defined (with these nice select boxes)

Correct me, if it overlook something, but hook_help() is generally for providing non-context-sensitive help.

Yeah you're right - Drupal's help wouldn't support adding help to node/x/map or admin/content/node-type/x/map, right?

#22, Aron: I agree with all your points. I've reviewed #18, did some minor adjustments (form layout wise and some whitespace removal) and updated tests to reflect changes.

Committed.

Thank you everybody!

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

Regarding the "many to many" solution - will this be achievable with the 2x version? I would like to be able to map a single field from csv to both a cck field AND a taxonomy term for that item.

Thanks,