Once #433388: Make mappings exportable with CTools/export is committed, we should refactor mapping pages so that you can create as many mappings as you want on /map and then pick on one active mapping.

CommentFileSizeAuthor
#18 542260-18_multiple_mappings.patch35.18 KBalex_b
#16 542260-15_multiple_mappings.patch34.69 KBaron novak
#12 542260-12_multiple_mappings.patch34.42 KBaron novak
#9 542260-7_multiple_mappings.patch30.62 KBaron novak
#6 542260-6_multiple_mappings.patch30.63 KBaron novak
#5 542260-5_multiple_mappings.patch23.9 KBaron novak
#2 542260_multiple_mappings.patch3.43 KBaron novak
#1 Picture 1.png34.11 KBalex_b

Comments

alex_b’s picture

alex_b commented
StatusFileSize
new Picture 1.png34.11 KB

Attached a wireframe on how the new node/id/map and admin/content/node-type/id/map pages could look like.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 542260_multiple_mappings.patch3.43 KB

#1 is a good goal, however to forge patches, design starts at the bottom, not on the top :)
This change affects also the very deep of the module, the schema (and if schema changes, it's a huge change usually :) )
So in this patch (what is only a code form of my ideas) shows how i imagine to modify the schema and the way how we load the mapping for the given node or content-type.

Fortunately it seems that the fact that the node mappings are not exportable does not really affect this question. In this case there won't be a default mapping ever on node overview pages, that's all.

With this way, there will be a coherent way to edit mappings: map/$mid/edit or something like this

alex_b’s picture

alex_b commented

'Stores the mappings for the feedapi mapper' => 'Stores mappings for the feedapi mapper'

+      'mid' => array(
+        'description' => 'The primary identifier for a mapping.',
+        'type' => 'serial',
+        'unsigned' => TRUE,
+        'not null' => TRUE),
+      'name' => array(
+        'description' => 'Human-readable identifier for a mapping',
+        'type' => 'varchar',
+        'length' => 256,
+	'not null' => TRUE),

I think there should be only one string id called 'id'. No serials, they are toxic :p

We should stick with a naming convention of 'mapping set' for a collection of single 'mappings'. 'mapping sets' can be associated with a node or a content type.

'mid from {feedapi_mapper} or -1 to use the exported code'

Can't say I like the magic number here. feedapi_mapper_active won't be exportable, right? What if we add a default_param to feedapi_mapper? We will populate it with the entity the mapper is created on. This default_param will also be exported. If feedapi_mapper looks for a mapping set, it will first look at _active.param, if there is no entry, it will look for feedapi_mapper.default_param. Can't say I'm completely excited about this approach, but I can't think of anything better right now.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

The string of name be the primary key, as long as this string has to be site-wide unique. This is maybe not a big restriction.

Yeah, magic number is not elegant.
What if we simply say if there is no entry in the feedapi_mapper_active table, let's use the mapping set from code?

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Active » Needs work
StatusFileSize
new 542260-5_multiple_mappings.patch23.9 KB

This patch contains an as-is version of the multiple mappings feature.
Here is a good point to collect what is done and what is missing.

Done
Changed DB schema
Implemented shiny overview form
Ability of creating new mapping sets
New, much simplier menu pathes
Mapping form and related functions were changed according to the new DB schema and menu pathes
The mapping is functional, all the backend of the module works fine

To do
Comments on the code
Fix mixed map, mapping terminology
Ability to delete a mapping set
Handling per-node, per-content-type overrides again (Valid on section is missing)
Provide UI to go back from map/%map path to the node/%node/map or content-type mapping overview page
Rename map_load() function. A better name is needed for the menu object loader placeholder
Ability of copying the mapping

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 542260-6_multiple_mappings.patch30.63 KB

Some improvements were done since #5.

Done
Changed DB schema
Implemented shiny overview form
Ability of creating new mapping sets
New, much simplier menu pathes
Mapping form and related functions were changed according to the new DB schema and menu pathes
The mapping is functional, all the backend of the module works fine
Comments on the code
Fix mixed map, mapping terminology (but this appears elsewhere in the module, i did not mess up other parts)
Ability to delete a mapping set
Provide UI to go back from map/%map path to the node/%node/map or content-type mapping overview page
Rename map_load() function. A better name is needed for the menu object loader placeholder
Ability of copying the mapping

To do

Handling per-node, per-content-type overrides again ('Valid on' section is missing)
SimpleTest tests surely fail. Needs to update the tests.
Provide better UI to relocate mappings while copying. I imagine as an autocomplete text fields. Any better idea?

alex_b’s picture

alex_b commented

#6: does not apply cleanly to CVS 2.x

feedapi_mapper_delete_full_form() -> should be feedapi_mapper_delete_set_form()

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

#6: Yeah, it does not apply because of the latest commit from you : http://drupal.org/project/cvs/211736
I will reroll asap.

delete_set_form - what is the meaning here?

delete_full_form just suggests that it deletes the full mapping. Or we can say entire here

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 542260-7_multiple_mappings.patch30.62 KB

Rerolled. Now applies cleanly.

alex_b’s picture

alex_b commented

#8 ;-)

"full mapping" - I thought we agreed on "mapping set" ?

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

yes, i use "mapping set" consequently in the patch :) i just overlooked this one :)

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 542260-12_multiple_mappings.patch34.42 KB

Done after #9:
Handling per-node, per-content-type overrides again.
Tests fixed. (however i get 1 fail on taxonomy test, manually the taxonomy mapping works, so likely it's a problem in the .test files, needs further investigating)
'Valid on' box appeared
delete_full became delete_set

To do
Decide if Relocate is needed and if so, how should it work
Fix that 1 fail feedapi_mapper_taxonomy.test

alex_b’s picture

alex_b commented

I get this error when installing:

user warning: Specified key was too long; max key length is 767 bytes query: CREATE TABLE feedapi_mapper ( `id` VARCHAR(256) NOT NULL, `default_param` VARCHAR(32) NOT NULL, `mapping` LONGTEXT NOT NULL, `unique_elements` LONGTEXT NOT NULL, PRIMARY KEY (id) ) /*!40100 DEFAULT CHARACTER SET UTF8 */ in /opt/local/apache2/htdocs/d6/includes/database.inc on line 515.

In #9, hook_update_N() did not work. Did not retest #12.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

"hook_update_N()" - there is no update hook for these changes at all. I found this confusing.

It seems that the max key length is variable between storage engines or mysql versions. I think it won't hurt if the length of the id will be 64 chars.

alex_b’s picture

alex_b commented

I think VARCHAR(256) should be VARCHAR(255), then it will work.

There needs to be an update hook for the schema changes, no?

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 542260-15_multiple_mappings.patch34.69 KB

"Prefix lengths are storage engine-dependent (for example, a prefix can be up to 1000 bytes long for MyISAM tables, 767 bytes for InnoDB tables)" - from http://dev.mysql.com/doc/refman/5.0/en/create-index.html

So 64 char length for the id seems to be fine.

To do
Decide if Relocate is needed and if so, how should it work
Fix that 1 fail feedapi_mapper_taxonomy.test
Make sure that the navigation flow is seamless (technically: drupal_goto always points to the correct place)

alex_b’s picture

alex_b commented

Ok, this is shaping up. I did a review and some modifications:

- Fixed redirects to node types with _
- Load all available mapping sets for given content type, no matter whether on node or not.
- Removed relocate mapping - this is for editing default_param, right? We should just populate default_param with the node type a mapping has been created on.
- Eliminated 'set active' check boxes on create and copy forms. Active is now set by a fall back to first available.

Todo:

- Eliminate direct calls to ctools_export_load_object outside of feedapi_mapper_load().
- Fix paths: map/add/feed_data, map/[name] - Not 100 % decided here. I would argue that we need 2 entry points for each form, one under content type, one
under node. Otherwise we will always leave our context and that's weird.
- Fix per node overriding

alex_b’s picture

alex_b commented
StatusFileSize
new 542260-18_multiple_mappings.patch35.18 KB

Forgot to attach patch.

alex_b’s picture

alex_b commented

Aron - before you do any other work on this, can we talk?

alex_b’s picture

alex_b commented
Status: Needs work » Postponed

I postpone this after conferring with Aron: it turns out that doing multiple mappings overly complicates the user interface.

I originally thought multiple mappings would be a good way to overcome the limitation of only one default mapping per content type, but this improvment can't be justified with an overly complex user interface.