[CLEAN-UP] Improve response dictionaries by remove duplicate code

[TiBiBa]

Description Currently we make a distinction between returns with an achievement and without in some places. This makes sense, as we only want to return an achievement when one is actually achieved. However, the rest of the response is duplicate coding in a lot of places. Take the following example:

if self.achievements.verify_save_achievements(user['username'],'adventure_name' in body and len(body['adventure_name']) > 2):
      return jsonify({'message': gettext('save_success_detail'), 'name': body['name'], 'id': program_id, "achievements": self.achievements.get_earned_achievements()})
return jsonify({'message': gettext('save_success_detail'), 'name': body['name'], 'id': program_id})

The complete return is 90% the same coding, which is a starting point for unfindable bugs. I would propose to re-structure all response using a dictionary that we dynamically fill with key/value pairs dependent on the request flow. For example:

response = {}
if achievement:
    response['achievement'] = 'bla bla bla';
response['message'] = 'This all went well';
return jsonify(response)

This happens in a lot of places so takes quite some work to clean-up, but I think this is worth it in the end.

[Annelein]

@Felienne could it be that this is old practise which has been removed throughout the years? I can't find anything like the example above.

Or do we want to change everything like this, to the usage of a dictionary as well?

        return jsonify({'filename': 'Micro-bit.py', 'microbit': True}), 200
    else:
        return jsonify({'message': 'Microbit feature is disabled'}), 403 

[Felienne]

Here too I am unsure! I have asked @rix0rrr

[Annelein]

@rix0rrr @Felienne would it make sense to also discuss https://github.com/hedyorg/hedy/issues/4029#issue-1583019171 :

• Remove all hard-coded response messages, even when not likely to be returned to user

And what about

• Replace all jsonify() returns to make_response() with the corresponding response code

[rix0rrr]

I wonder if I have removed some of the duplication of achievement code myself already.

I believe the way it works right now is:

• Newly earned achievements are accumulated into the user's session (whenever an achievement it earned, it gets added to an array). This array behaves like a "queue" of achievements that still need to be sent to the front-end, to be displayed to the user.
• This array is emptied whenever the list is sent to the frontend;
  • Either as part of a request that returns a payload that happens to be able to include achievements
  • Or in response to a periodic query by the frontend.

Remove all hard-coded response messages, even when not likely to be returned to user

I would have a good long think about doing this. The more strings you stick into "translatable" territory, the more work you make for translators. They are doing us a favor with their unpaid labor... I'd hate to ask them to spend hours translating strings that no one will ever see.

If the error should never happen in normal operation, but indicates a contract mismatch between the frontend and backend code... I think I wouldn't bother.

Replace all jsonify() returns to make_response() with the corresponding response code

I'm not entirely sure if or whether this is better. From the Flask documentation of make_response it seems more like a function that should be called by middleware.

I think what we are expected to do as Flask app builders is return a value that will be the input to make_response (like a (body, status) tuple).

But I don't have a strong opinion either way.