Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Set the default behavior, in case people don't have core.autocrlf set.
* text=auto eol=lf
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,4 @@ student-work/
.idea/
.byebug_history
coverage/
.vscode
_history
3 changes: 3 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"files.eol": "\n"
}
4 changes: 4 additions & 0 deletions Gemfile.lock
Copy link

@martindolores martindolores Mar 26, 2025

Choose a reason for hiding this comment

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

Not 100% sure if this file (Gemfile.lock) should be committed - please double check this

Copy link
Author

Choose a reason for hiding this comment

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

The changes in Gemfile.lock weren't intentionally added by me - they were automatically generated when I ran the app locally. These changes add x86_64-Linux platform support alongside the existing aarch64 platform.

From what I understand, this is generally beneficial as it expands compatibility to both architectures, but I wanted to confirm if this aligns with our project standards before committing these changes. If preferred, I can revert these Gemfile.lock changes.

Choose a reason for hiding this comment

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

What you say seems good to me! Lets double check with Nebula tomorrow during standup but that sounds fine :)

Copy link

Choose a reason for hiding this comment

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

These packages are present in upstream

I recommend merging your branch with 8.0.x and changing the base branch on this PR to 8.0.x

Copy link
Author

Choose a reason for hiding this comment

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

Hi @b0ink I have created a new pull request as suggested from 8.0.x branch. https://github.com/thoth-tech/doubtfire-api/pull/55/files

Copy link

@martindolores martindolores Apr 3, 2025

Choose a reason for hiding this comment

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

Hi @amriith 😄, you can changes the base branch on this PR as so without having to open up a new PR:
image
image

It also looks like some files that you have removed has been added in and some merge conflicts on the other PR:
image

I suggest to just close the other PR and change the base branch on this to 8.0x. Note that you will have to merge 8.0x to this branch (just run git merge 8.0x and fix any conflicts)

Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ GEM
faraday-net_http (3.1.0)
net-http
ffi (1.17.0-aarch64-linux-gnu)
ffi (1.17.0-x86_64-linux-gnu)
fugit (1.11.0)
et-orbi (~> 1, >= 1.2.11)
raabro (~> 1.4)
Expand Down Expand Up @@ -265,6 +266,8 @@ GEM
nio4r (2.7.3)
nokogiri (1.16.5-aarch64-linux)
racc (~> 1.4)
nokogiri (1.16.5-x86_64-linux)
racc (~> 1.4)
observer (0.1.2)
orm_adapter (0.5.0)
parallel (1.24.0)
Expand Down Expand Up @@ -490,6 +493,7 @@ GEM

PLATFORMS
aarch64-linux
x86_64-linux

DEPENDENCIES
better_errors
Expand Down
4 changes: 2 additions & 2 deletions app/api/users_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ class UsersApi < Grape::API
change_self = (params[:id] == current_user.id)

params[:receive_portfolio_notifications] = true if params.key?(:receive_portfolio_notifications) && params[:receive_portfolio_notifications].nil?
params[:receive_portfolio_notifications] = true if params.key?(:receive_feedback_notifications) && params[:receive_feedback_notifications].nil?
params[:receive_portfolio_notifications] = true if params.key?(:receive_task_notifications) && params[:receive_task_notifications].nil?
params[:receive_feedback_notifications] = true if params.key?(:receive_feedback_notifications) && params[:receive_feedback_notifications].nil?
params[:receive_task_notifications] = true if params.key?(:receive_task_notifications) && params[:receive_task_notifications].nil?

# can only modify if current_user.id is same as :id provided
# (i.e., user wants to update their own data) or if update_user token
Expand Down
15 changes: 15 additions & 0 deletions app/models/task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ def specific_permission_hash(role, perm_hash, _other)
delegate :update_task_stats, to: :project

after_update :update_task_stats, if: :saved_change_to_task_status_id? # TODO: consider moving to async task
after_update :send_feedback_notification, if: :saved_change_to_task_status_id?

validates :task_definition_id, uniqueness: { scope: :project,
message: 'must be unique within the project' }
Expand Down Expand Up @@ -1362,6 +1363,20 @@ def read_file_from_done(idx)
nil
end

def send_feedback_notification
return unless task_status.in?([TaskStatus.redo, TaskStatus.fail, TaskStatus.fix_and_resubmit, TaskStatus.feedback_exceeded, TaskStatus.discuss, TaskStatus.demonstrate, TaskStatus.complete])
return unless project.student.receive_feedback_notifications
return unless unit.send_notifications

begin
logger.info "Checking feedback email for project #{project.id}"
logger.info "Emailing feedback notification to #{project.student.name}"
PortfolioEvidenceMailer.task_feedback_ready(project, [self]).deliver
rescue => e
logger.error "Failed to send feedback notification email. Error: #{e.message}"
end
end

private

def delete_associated_files
Expand Down
23 changes: 15 additions & 8 deletions config/environments/development.rb

Choose a reason for hiding this comment

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

This is a config file - changes to this are typically not something you would commit unless they are part of a necessary configuration

If testing your changes, just leave in local repo without committing i.e. dont include in this PR

Copy link
Author

Choose a reason for hiding this comment

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

I've removed the configuration file from this PR as suggested, Thanks for letting me know.

Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,21 @@
# Store uploaded files on the local file system (see config/storage.yml for options).
# config.active_storage.service = :local

# Don't care if the mailer can't send.
config.action_mailer.raise_delivery_errors = false

config.action_mailer.perform_caching = false

# Tell Action Mailer not to deliver emails to the real world.
# Write them to file instead (under doubtfire-api/tmp/mails)
config.action_mailer.delivery_method = :file
# Don't care if the mailer can't send.
config.action_mailer.raise_delivery_errors = true

config.action_mailer.perform_caching = false

# Tell Action Mailer not to deliver emails to the real world.
# Write them to file instead (under doubtfire-api/tmp/mails)
config.action_mailer.delivery_method = :file
config.action_mailer.file_settings = {
:location => File.join(Rails.root, 'tmp', 'mails')
}

# Add more verbose logging for ActionMailer
config.action_mailer.logger = Logger.new(STDOUT)
config.action_mailer.logger.level = Logger::DEBUG

# Print deprecation notices to the Rails logger.
config.active_support.deprecation = :log
Expand Down