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

refactor(Controllers): Pass in domain instead of Long to methods #65

Merged

Conversation

hirokiterashima
Copy link
Member

Changes

  • Added ProjectRepository and WorkgroupRepository
  • Updated tests

Test

  • Student VLE
    • save achievement (e.g. completing c-rater milestone item should enter new row in db)
    • save notification: post reply to discussion post
    • get notification: open vle
    • dismiss notification
    • Notebook
      • get/create/edit notebook items
  • CM
    • retrieve achievement (e.g. open teacher report view)
  • AT
    • getAuthorProjectConfig (load AT)
    • notifyAuthorBeginEnd (open/close project authoring)
    • export project
    • copyProject
    • saveProject

Closes #64

- Added ProjectRepository and WorkgroupRepository

Closes #64
Copy link
Member

@geoffreykwan geoffreykwan left a comment

Choose a reason for hiding this comment

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

Get notification when the student opens the VLE doesn't work anymore. I think this is because the logic was changed in NotificationController.getNotifications().

Before when it was working there is an "else if".

    if (toWorkgroupId != null) {
      Workgroup workgroup = workgroupService.retrieveById(new Long(toWorkgroupId));
      if (isStudentAndNotAllowedToSaveNotification(user, run, workgroup)) {
        return new ArrayList<Notification>();
      }
    } else if (!user.isAdmin() && !runService.hasRunPermission(run, user, BasePermission.READ)) {
      return new ArrayList<Notification>();
    }
    return vleService.getNotifications(id, runId, periodId,
        toWorkgroupId, groupId, nodeId, componentId);

The changed code that doesn't return the notifications has the "else if" changed to "if".

    if (toWorkgroup != null) {
      if (isStudentAndNotAllowedToSaveNotification(user, run, toWorkgroup)) {
        return new ArrayList<Notification>();
      }
    }
    if (!user.isAdmin() && !runService.hasRunPermission(run, user, BasePermission.READ)) {
      return new ArrayList<Notification>();
    }
    return vleService.getNotifications(id, run, periodId, toWorkgroup, groupId, nodeId,
        componentId);

Copying a project in the Authoring Tool shows a popup error but the project still gets copied. This popup error does not happen on develop or PROD. I couldn't figure out why the popup error is happening.

All the other endpoints I tested seemed to work.

- Reverted NotificationController logic change and copyProject
  prototype.
@hirokiterashima
Copy link
Member Author

I reverted the logic changes in NotificationController.getNotifications() and also the changes in AuthorAPIController.copyProject. I think this problem may be fixed by refactoring the the call to projectService.copyProject(projectId, user); to take in a Project object. I'll work on this in a separate branch.

Copy link
Member

@geoffreykwan geoffreykwan left a comment

Choose a reason for hiding this comment

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

Looks good.

@geoffreykwan geoffreykwan merged commit 8681647 into develop Nov 29, 2021
@geoffreykwan geoffreykwan deleted the issue-64-pass-in-domain-objects-to-controller-methods branch November 29, 2021 21:43
hirokiterashima added a commit that referenced this pull request Nov 30, 2021
- The user object retrieved in the controller by DomainClassConverter
is now a HibernateProxy object due to PR #65, so I updated the User's
equal object to check for it.
geoffreykwan pushed a commit that referenced this pull request Nov 30, 2021
- The user object retrieved in the controller by DomainClassConverter
is now a HibernateProxy object due to PR #65, so I updated the User's
equal object to check for it.
@geoffreykwan
Copy link
Member

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Pass in domain objects to Controller method params instead of Long runId
2 participants