Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate the course assignment table according to researchers' preferences #10

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

SamuelVch98
Copy link
Collaborator

Integration of the handsontable table into the application.
Ability to save the state of a table, delete data, export it as a CSV file and publish it.
Update README

templates/assignment.html Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
assignment.py Outdated Show resolved Hide resolved
assignment.py Show resolved Hide resolved
assignment.py Outdated Show resolved Hide resolved
static/scripts/index.js Outdated Show resolved Hide resolved
static/scripts/index.js Outdated Show resolved Hide resolved
static/scripts/index.js Outdated Show resolved Hide resolved
static/scripts/index.js Outdated Show resolved Hide resolved
templates/assignment.html Outdated Show resolved Hide resolved
static/scripts/assignment_table.js Outdated Show resolved Hide resolved
static/scripts/assignment_table.js Outdated Show resolved Hide resolved
static/scripts/assignment_table.js Outdated Show resolved Hide resolved
@anthonygego
Copy link
Member

Can you rebase ?

Copy link
Member

@anthonygego anthonygego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can change the preferences line and put whatever we want in the cells, and then save the table and loose the actual prferences...

Also, I think that you should add some short notice on how to use the table: which cells need to be modified, how....

static/scripts/assignment_table.js Outdated Show resolved Hide resolved
static/scripts/assignment_table.js Outdated Show resolved Hide resolved
@anthonygego
Copy link
Member

Can you fix conflicts and rebase ?


rank = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this piece of code comes from #42. We'll wait for it to be merged before proceeding further with this one.

toastNotification.show();
})
$('#button-create-assignments').click(function () {
storeDataLocally(data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want this ? Storing this in the local storage only allows the local browser to load saved data. This means:

  • an admin currently working on assignments should keep using the same browser until assignments are published
  • if an admin save their own assignments, another admin cannot get the current state of assignments and go on until the data are ready for publishment.

Data should be saved in remote database. And can only be viewed by teachers and researchers once they are published.

def load_data():
current_year = get_current_year()
courses = (db.session.query(Course).filter_by(year=current_year).order_by(Course.quadri).all())
users = db.session.query(User).filter_by(active=True).all()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beware: if you save assignments and then de-activate a user, their data won't be found when reloading the assignment table. You should check for this.

}

// Verify if the table is modified locally
retrieveDataLocally();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either:

  • remove the function definition as it is used once and alter the value of a variable out of its scope (data)
  • make the function return the loaded value.

// Old non-empty value and new non-empty value: loadValue++ and nbrAssistants++.
// Old null value and new non-empty value: loadValue++ and nbrAssistants++.
// Old non-null value and new null value: loadValue-- and nbrAssistants--.
if (oldValue !== newValue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need a small doc for this in the UI. It won't be clear for user that whatever the cell content is corresponds to assigned and that an empty cell corresponds to not assigned.

Another nice and valid solution would be to catch double click for instance and check or uncheck the cell.

Comment on lines +43 to +45
export const taLoad = 4; // Load of a teaching assistant
export const phdLoad = 2; // Load of a phd student
export const postDocLoad = 1; // Load of a post doc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already defined in enums.py. You should rather inject them dynamically from the template instead of duplicating definitions.

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