After created an refund order , trying to update I checked the order don't save or lose some information as owner uid, created date, etc. I checked the form structure and we are not loading the correct created time.

Also I checked the implementation and you have part of your code similar with DrupalCommerceOrder, as the save function, but in other parts, as to the forms validation and submit you are using a different a approach. Which implementation pattern are you using for this module? If you let me know It will be easier to me create a patch that you can accept for fix those issues.

Thanks,

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dhernandes’s picture

Issue summary: View changes
dhernandes’s picture

Hi did some changes in the module to fix the problems I was folding regarding the issue reported. I am attached the patch here in case you think you could use it.

The changes I've implemented are:

  • Saving a commerce_return object when call the commerce_return_create to avoid database garbage ( when you create an commerce_return , even without saving in the database, the code was saving all the customer_profile and commerce_line_items in db).
  • Fixed losing information when updated a return: email, return owner, hostname, created date, return status, return reason for each line item.
  • Added user and email validation for the return form.
  • The module interface was not loading the correct user and e-mail it is always showing the current logged in user.
  • Fix the creation process to avoid duplicate entries for customer_profile.
  • Avoid to create a new return object for an order which already has an return.
GoZ’s picture

I can't reproduce. Can you explain how to reproduce issue ?

What i check :
- Create client account. Make and order with client account.
- With admin (super admin), update order to complete status.
- With client, create return order.
- With admin, check order (all is all right), update order (client name of shipping informations. All datas are corrects.

Make same test creating return with admin, and no issues.

agoradesign’s picture

I can reproduce this very easily:

I'm using Entity API 1.5 with Drupal 7.34 and the current dev version (7.x-2.0-rc2+15-dev) of Commerce RMA.

I did have problems with dev and stable, when I tried the module about one month ago. The patch seemed to solve my problems. I'm not sure, which properties were lost after update. However, today I've tried Commerce RMA again and installed the current dev version. I did the following steps:

1. create a return order with the client.
2. As for my installation "pending" status is also among the allowed ones, I went directly as admin to the edit page of the return entity and simply just pressed the save button - one time I tried with and one time without creating a revision.
3. No matter, if I chosse to create a revision or not, the create date of the return entity is always lost (to be exact: the value of the create date is set to '8')

The patch from #2 unfortunately does not apply anymore...

PS: it doesn't make a difference, if I leave the date field of the edit form empty or not

agoradesign’s picture

Status: Active » Needs review
FileSize
6.58 KB

I've re-rolled the patch from #2 to work with the latest dev, and modified a few things. Here are my changes (in comparison to the original patch):

Saving a commerce_return object when call the commerce_return_create to avoid database garbage ( when you create an commerce_return , even without saving in the database, the code was saving all the customer_profile and commerce_line_items in db).

I dropped this for several reasons:
1. I couldn't reproduce this at all
2. Even if I could, this wouldn't be an appropriate fix at all. Without saving the return explicitely, nothing - no return, no line items, etc - should be saved!

Avoid to create a new return object for an order which already has an return.

I dropped this because this has nothing to do with the original issue. You should open a new issue instead. Mainly, this is a architectural decision, the module owners have to decide. Imho there's nothing wrong about having the possibility to create more than one return per order. And even if you want to prevent this, running a query within commerce_return_new_order_wrapper() function is definitely the wrong place. Because if you change it like that, the function won't longer do what it should do.

In the form validation, the original patch accidentially dropped the call to field_attach_form_validate(), which I re-included (this is also included in the commerce_order module code)

An open question are the removed lines in commerce_return_form() (in commerce_return.admin.inc). On the one side, commerce_order also doesn't have this else-branch (that's, why the patch removes it), on the other side, I don't think it's bad or harmful. Nevertheless, my patch removes the lines, like it was in the original patch.

Summarized, this patch now only adjusts code parts (form validation, submit + a small change in an if condition inside the save function) to match exactly the approach, how these things are handled in commerce_order. This helps losing date information on updating an order.

jkuma’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6 KB

Summarized, this patch now only adjusts code parts (form validation, submit + a small change in an if condition inside the save function) to match exactly the approach, how these things are handled in commerce_order. This helps losing date information on updating an order.

Ok, fair enough, thanks for the hard work here, the code looks cleaner and it fixes up the raised issue. I've remove some trailing whitespace and some line codes that are no longer needed. I'm updating this issue to RTBC.

  • goldorak committed 889c759 on 7.x-2.x authored by agoradesign
    Issue #2378497 by dhernandes, agoradesign, goldorak: Losing infromation...
jkuma’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thank you guys!

Status: Fixed » Closed (fixed)

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