When you add new line items to an order, there's no visual indication that the entire form must be saved for the line items to be saved via the line item reference field. This might not be a big deal, since we might want to convert away from using that widget entirely... but in the meantime, we should consider adding a blocks / fields UI style warning indicating the form needs to be submitted to preserve changes.

CommentFileSizeAuthor
#2 1059400.patch763 bytesartusamak

Comments

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Issue tags: +dcparisprintmarch

Tagging

artusamak’s picture

artusamak
he/him
Primary language French
Location Bzh
commented
Status: Active » Needs review
StatusFileSize
new 1059400.patch763 bytes

Here a small patch doing the trick.

recidive’s picture

recidive commented
Status: Needs review » Reviewed & tested by the community

Looks good:

https://skitch.com/recidive/r1cux/order-3-drupal-commerce

It can be picked from this commit:

https://github.com/recidive/drupalcommerce/commit/c4898f616823e491195814...

rszrama’s picture

rszrama commented
Status: Reviewed & tested by the community » Needs work

Currently this patch shows the warning as soon as the "Add line item" button is clicked showing which type of line item to add to the order. However, we only need to show it when a line item is actually added to the order - i.e. the "Add product" button is clicked. We can't rely on that, though, because there might be different buttons for different types of line items...

I think perhaps what should happen is when a line item is actually saved, we should toggle a boolean in the $form_state indicating the message should be displayed. This will be preserved regardless of what other buttons are clicked. This code should go somewhere near the instance of commerce_line_item_new() around line 1100. Then the original patch should be updated to use this boolean... perhaps just name it $form_state['line_item_save_warning'].

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented

Repo: git.drupal.org:sandbox/pcambra/1081200.git
Branch: 1059400
Diff: http://drupalcode.org/sandbox/pcambra/1081200.git/commitdiff/0533e39ea60...

recidive’s picture

recidive commented
Status: Needs work » Needs review 
if (!empty($form_state['line_item_save_warning']) && $form_state['line_item_save_warning'] == TRUE)

Isn't it better to use isset() rather than !empty() in this case? I guess it will not have any practical issues but the following code looks more appropriate IMO:

if (isset($form_state['line_item_save_warning']) && $form_state['line_item_save_warning'])
rszrama’s picture

rszrama commented
Status: Needs review » Fixed

Given the usage in Drupal as a whole, it's probably enough to just have the single condition being either

if (!empty($form_state['line_item_save_warning']))

See for example the various uses of is_new in the core modules. It'll pass the PHP notice if the key doesn't exist, and it'll ensure it's not FALSE. I'll flatten the statement to this and commit.

Thanks for the work / review on this guys. : )

Status: Fixed » Closed (fixed)
Issue tags: -dcparisprintmarch

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

Issue tags: +dcparisprintmarch

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.