Codecov Report

Merging #35221 into release-10.2.0 will decrease coverage by <.01%.
The diff coverage is 12.5%.

@@                 Coverage Diff                  @@
##             release-10.2.0   #35221      +/-   ##
====================================================
- Coverage             64.39%   64.39%   -0.01%     
  Complexity            20049    20049              
====================================================
  Files                  1285     1285              
  Lines                 76842    76844       +2     
  Branches               1308     1308              
====================================================
  Hits                  49480    49480              
- Misses                26978    26980       +2     
  Partials                384      384

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24e0e79...b54f30c. Read the comment docs.

@VicDeo Please do not merge until the decisions about the release are made.

I have tested this and it works fine on the frontend.
But network response tab have two status
{"ocs":{"meta":{"status":"ok","statuscode":200,"message":"Couldn't send mail to following recipient(s): merouser3 ","totalitems":"","itemsperpage":""},"data":{"status":"error"}}}
status "ok" and status "error".

Is this the expected behavior?

@paurakhsharma
What was the behavior with 10.1 ? can you please double check

@paurakhsharma
What was the behavior with 10.1 ? can you please double check

I double checked and this is the network response in 10.1 {"data":{"message":"Couldn't send mail to following recipient(s): arkouser "},"status":"error"}

@VicDeo would be good to also update the frontend unit tests. I believe we have some for the email part.

backend unit tests as well... but here I believe this is not coverable...

please always add statements and reasons if you decide to skip unit tests

@paurakhsharma
What was the behavior with 10.1 ? can you please double check

I double checked and this is the network response in 10.1 {"data":{"message":"Couldn't send mail to following recipient(s): arkouser "},"status":"error"}

@VicDeo so this changes the response format ? I thought this PR was an attempt to bring it back to what it was before

@PVince81 to bring it back I need to kill the controller and send the response from core/ajax/share.php.
Frontend tests are adjusted already.

not coverable as I can't mock Share::setSendMailStatus properly

10.1 sends a response with \OCP\JSON::success or \OCP\JSON::error from core/ajax/share.php

#33180 changes it send a response from OCA\Files_Sharing\Controller\Share20OcsController class via OCS

that's why the responses differ - one is pure AJAX and other is OCS

@VicDeo would you qualify this as API change ? or would the new format be solidified as a public API if it's OCS ?

the old API as far as I remember was not OCS so considered private

@michaelstingl I also assume no client was using this private API in ajax/share.php

@michaelstingl I also assume no client was using this private API in ajax/share.php

Is this the mail for public links? Never made it to the clients because of the missing proper API.

@PVince81 it didn't look like a public API at all as it needed logged in user and a valid CSRF token:
https://github.com/owncloud/core/blob/master/core/ajax/share.php#L40-L41

{
  "ocs":
  {
      "meta": 
      {
           "status":"ok",
           "statuscode":200,
           "message":"Couldn't send mail to following recipient(s): merouser3 ",
           "totalitems":"",
           "itemsperpage":""
      },
     "data":
     {
       "status":"error"
     }
   }
} 

meta.status is HTTP-alike OCS status. ok here is just a text test representation of a statuscode 200
while data.status is exactly the same with the prev implementation. Prev implementation used deprecated \OCP\JSON public API to deal with frontend - it's success and error methods respectively