[10.2.0] Treat send attempt as unsuccessful when there is a message.

[VicDeo]

Backport of #35220

Description

result.ocs.meta.status is 200 when the email was not sent.

Related Issue

• Fixes Sharer gets "Email notification was sent!" message even when the email was not sent #35218

Motivation and Context

Frontend says that email is successfully sent even when it has been not sent.

How Has This Been Tested?

See #35218

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

[codecov]

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

Flag	Coverage Δ	Complexity Δ
#javascript	53.66% <100%> (ø)	0 <0> (ø)	⬇️
#phpunit	65.53% <0%> (-0.01%)	20049 <0> (ø)

Impacted Files	Coverage Δ	Complexity Δ
...es_sharing/lib/Controller/Share20OcsController.php	87.25% <0%> (-0.32%)	202 <0> (ø)
core/js/sharedialogshareelistview.js	80.48% <100%> (ø)	0 <0> (ø)	⬇️

---

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.

[micbar]

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

[paurakhsharma]

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?

[patrickjahns]

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

[paurakhsharma]

@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"}

[PVince81]

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

[PVince81]

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

[PVince81]

@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

[VicDeo]

@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

[VicDeo]

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

[PVince81]

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

[PVince81]

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

[PVince81]

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

[michaelstingl]

@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.

[VicDeo]

@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

[VicDeo]

{
  "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

[PVince81]

👍