Skip to content

Conversation

@lainets
Copy link
Contributor

@lainets lainets commented Jun 5, 2023

I created this PR to discuss the idea.

I do not think it is a good idea to load pages using ajax and then use DOM manipulation to get the needed information out of the loaded page. This will easily cause issues (that are also hard to notice and debug) if the page HTML changes. There is also no indication in the HTML itself (or whatever creates it) that the JS is dependent on a specific structure. I propose we add JSON versions of the pages that would otherwise need such DOM introspection to move the logic to python where it will be much easier to see if changes have an effect on something else.

Description

What?

[ANSWER HERE]

Why?

[ANSWER HERE]

How?

[ANSWER HERE]

Testing

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

[ADD A DESCRIPTION ABOUT WHAT YOU TESTED MANUALLY]

Did you test the changes in

  • Chrome
  • Firefox
  • This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

Programming style

  • Did you follow our style guides?
  • Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

  • documents inside the doc directory.
  • README.md.
  • Aplus Manual.
  • Other documentation (mention below which documentation).

Is it Done?

  • Reviewer has finished the code review
  • After the review, the developer has made changes accordingly
  • Customer/Teacher has accepted the implementation of the feature

@markkuriekkinen
Copy link
Contributor

This sounds like a big change and it is hard to grasp beforehand how much this will require changes.

The existing exercise_plain.html and submission_plain.html templates render also the points nav bar at the top of the embedded exercise frame in the chapter page. Would you then render the points nav bar also in the server and include it in the JSON response or would you update the nav bar in the frontend JavaScript?

There are also some AJAX exercises that update the points via a JS event handler. AJAX exercise means that the submission is not posted directly to the A+ server. Normally, the submit form posts to the A+ server.

In this example, the JSON data is populated within the view method but I would change it such that the data is populated through inheritance and using a custom JsonEncoder to encode the models (like Submission).

How would a custom encoder work?

I didn't understand how the _messages.html change is related to the topic.

@lainets
Copy link
Contributor Author

lainets commented Jun 5, 2023

This sounds like a big change and it is hard to grasp beforehand how much this will require changes.

I wouldn't be changing all ajax calls right now. I would only change the ones used by active elements for now (submitting and getting a submission). This shouldn't need much changes.

The existing exercise_plain.html and submission_plain.html templates render also the points nav bar at the top of the embedded exercise frame in the chapter page. Would you then render the points nav bar also in the server and include it in the JSON response or would you update the nav bar in the frontend JavaScript?

It kind of depends on how it works. In general, it would be best to do the update without rendering on the server, i.e. just pass the data (points in this case) to the JS and let it do the update but I'd have to check how the code works before I can say for sure.

Another option is to fully delegate the points rendering to the client JS but I'd prefer an external framework like React for doing that. We could also look into gradually switching to React or some other similar framework but that is outside the scope of this PR as we need to get the current issue fixed.

There are also some AJAX exercises that update the points via a JS event handler. AJAX exercise means that the submission is not posted directly to the A+ server. Normally, the submit form posts to the A+ server.

The PR text is mainly about requests made to A+. It might be applicable to the ajax exercises too but I'm not that familiar with them, so can't comment.

How would a custom encoder work?

It's basically just a class inheriting the django encoder with an if isinstance statements in the encoding function (and super() call for other types). The output for each object would include fields that are needed by the ajax calls (e.g. the ones for submission in the example code, more can be added later if needed). It is very simple to implement. Git manager already has a simple one https://github.com/apluslms/gitmanager/blob/41e24405aad57efe8a235db05ac3178c0c655e56/util/export.py#L276

With a custom encoder, we could do stuff like JsonResponse({"submission": submission}) where submission is a Submission object.

I didn't understand how the _messages.html change is related to the topic.

It is about the concrete implementation of this on active elements. The active elements code displays the messages rendered by _messages.html. Earlier it would do DOM introspection to detect them but I changed the messages to be rendered separately on the server and in the JSON.

@markkuriekkinen
Copy link
Contributor

I thought it would be faster to fix the bug with small changes. You are suggesting large refactoring, but we should fix the active element submission feedback polling quickly. If we at least first finish a quick and small solution, then there is more time later on to think about major refactoring.

For example, the data-poll-url attribute is already used here:

var poll_url = content.find(".exercise-wait").attr("data-poll-url");

If data-ready-url were added there similarly, would that lead to a quick and simple fix to the urgent issue (#1188)?

@lainets
Copy link
Contributor Author

lainets commented Jun 6, 2023

Oh, and many views already have two versions: one for ajax request and one for normal requests. It makes much more sense to me to just return the data the client wants in a json instead of having them find it in the html. It also seems a bit dubious to serve a different page on an ajax request than a normal request (and more difficult to debug).

@lainets
Copy link
Contributor Author

lainets commented Jun 7, 2023

#1192 implements a quick fix to the issue as we need it fixed ASAP. Let's have this PR solely about the ajax refactoring

lainets added 3 commits June 7, 2023 12:49
Use `@accepts_mimes(<list of implemented mime types>)` to decorate view
method/function with the accepted types, and then use
`request.expected_mime` to get the type the client expects (the one that
matches best with the implemented mime types).
Also fixes an issue where the submission isn't visible if messages
are present (e.g. "Staff can submit assignments without enrolling.")
@lainets lainets changed the title WIP fix active elements WIP using JSON responses to AJAX requests Jun 7, 2023
@lainets
Copy link
Contributor Author

lainets commented Jun 7, 2023

I now made an implementation of JSON responses to AJAX calls for active elements submitting.

The more I think about this, the more I want to try swapping to using React because then most of the rendering happens on the client, simplifying updating of the site considerably. Edit: as an example, the alert messages could be renderer on the client based on a list of messages inside the JSON to allow easily updating the alerts instead of rendering them on the server like how it works currently and in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants