Conversation
andrey-canon
left a comment
There was a problem hiding this comment.
In general, the code looks good.
What really concerns me is the mixed functionality in the following endpoint:
/eox-nelp/api/programs/v1/program-lookup/
Originally, this returned the list of all programs.
On the other hand, we also have:
/eox-nelp/api/programs/v1/program-lookup/?national_id=1234567890
which returns the programs related to a specific user.
However, with these new changes, the endpoint:
/eox-nelp/api/programs/v1/program-lookup/
— as I understand it — will now start returning certificate information for the user making the request.
I personally see this endpoint as something not tied to any specific user, simply returning general program information.
That makes me think a small refactor would make more sense.
Suggested structure:
-
GET /eox-nelp/api/programs/v1/program-lookup/
→ Returns all programs. -
GET /eox-nelp/api/programs/v1/program-lookup/?query=
→ Returns all programs filtered by the query params (only for program filters). -
GET /eox-nelp/api/programs/v1/user-programs/<national_id or id>
→ Returns the programs for a specific user and can support the filters proposed in this PR.
Alternatively:
/eox-nelp/api/programs/v1/user-programs/all/?national_id=xxx
- If
national_idis not provided, we could return a Bad Request. - We could also return all results by default, but that might be excessive.
Personally, I prefer:
/eox-nelp/api/programs/v1/user-programs/<national_id or id>
But this should definitely be discussed with the client.
| Certificate_PATH = serializers.CharField(required=False, allow_null=True) | ||
| Certificate_URL = serializers.SerializerMethodField() |
There was a problem hiding this comment.
why are you keeping this invalid format ? why do you need the certificate path ?
ea12ec7 to
09354de
Compare
andrey-canon
left a comment
There was a problem hiding this comment.
could you please include the last field(completion_date) asked by the client ?
feat: update test with new change feat: add certificated_only test chore: quality adjustments chore: complexity lint improvement
09354de to
712527c
Compare
this only add the functionality for completion date. Test and lints would be fixed.
andrey-canon
left a comment
There was a problem hiding this comment.
LGTM and the basic functionality is working for me


Description
&certificated_only=trueJira story: https://edunext.atlassian.net/browse/FUTUREX-1491
Testing instructions
Screencast.from.17-10-25.10.54.47.webm
Certificate_URLfield works.