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.

Sign up for GitHub

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

Jump to bottom

Edit button #33

Merged
merged 9 commits into from
Feb 28, 2024
Merged

Edit button #33

merged 9 commits into from
Feb 28, 2024

Conversation

Prakhar-commits
Copy link
Contributor

Summary

This PR aims to add the delete functionality for quiz creator for editing sessions

  • Test Responsiveness
    • Laptop (1200px)
    • Tablet (760px)
    • Phone (320px)
  • Cross-Browser Testing
    • Chrome
    • Firefox
  • Local Language Support
  • Hygiene and Housekeeping
    • Self-review
    • Comments have been added appropriately
    • Check for bundle size here if adding a package
    • Added relevant details like Labels/Projects/Milestones etc.
    • If adding or removing any environment variable, update docs/ENV.md, .env.example and the Github workflows.
  • Testing
    • Wrote tests
    • Tested locally
    • Tested on staging
    • Tested on an actual physical phone
    • Tested on production
  • Lighthouse Checks
    • Images have alt attributes
    • Any <img> tags have width and height specified
    • Any target="_blank" links have rel="noopener"
    • Only SVGs are used as images. If PNGs are used, their size has been optimised.
    • Any SVG buttons without text have their aria-label attributes set

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/Session.tsx Outdated
});
console.log(props.FormData.test.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove later

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/Session.tsx
},
};
} else if (type === "edit" && sessionId) {
const FormData = await getASession(Number(sessionId));
Copy link
Contributor

@suryabulusu suryabulusu Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why are you doing getASession when that session data is already there with you ? why the additional instance.get() request

when you are "editing" a session, you already have that session data!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to get the data for the session with the particular id

Copy link
Contributor

Choose a reason for hiding this comment

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

but its already there. why get again?

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/api/PostFormData.ts Outdated
platform: test.platform,
platform_link: "",
portal_link: "", //responds to the portal link
name: test.name, //x
Copy link
Contributor

Choose a reason for hiding this comment

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

remove these //x comments. maybe copy paste elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/api/UpdateFormData.ts Outdated
import type { NextApiRequest, NextApiResponse } from "next";

/*
async function formatDateTime(date: string, time: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if its commented out, just remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/api/UpdateFormData.ts Outdated
}
*/

async function formatDateTime(date: string, time: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this code is already there in PostFormData ?

maybe you can have a "single" file for both PostFormData and UpdateFormData since there's so much similarity. avoid unnecessary code repetition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged into a single file

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/api/UpdateFormData.ts Outdated
sessionId: string
) {
const { student, test, timeline, session } = formData;
console.log(sessionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/api/UpdateFormData.ts Outdated
...(test.optionalLimit ? { optional_limits: test.optionalLimit } : {}),
...(test.markingScheme ? { marking_scheme: test.markingScheme } : {}),
...(test.type ? { test_type: test.type } : {}),
date_created: new Date().toISOString().split("T")[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

why? this would then get updated to current time.

its date_created .. not date_updated-- it is not supposed to change.

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/pages/api/UpdateFormData.ts Outdated
};

if (student || test || timeline) {
patchBody.meta_data = {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is shortened link? please check and ensure everything is there in metadata

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/utils/FormInputHandling.ts
return date.toISOString().split("T")[1].slice(0, 5); // Extracts the time part
}

async function getASession(id: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel this is unnecessary. have the function of course. but don't use it yet. its not needed for edit i feel

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
src/utils/PublishSnsMessage.ts Outdated
@@ -7,7 +7,7 @@ AWS.config.update({
});

const sns = new AWS.SNS();
function publishMessage(sessionId) {
export function publishMessage(sessionId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

publish message type is wrong.. it doesn't just take session id. it actually takes in a message object with a more involved structure.

the message object has: [action, id, patch_session] keys as of now. you can put this in types.

i am getting compilation error when i'm trying out on local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved now

suryabulusu
suryabulusu reviewed Feb 11, 2024
View reviewed changes
Copy link
Contributor

@suryabulusu suryabulusu left a comment

Choose a reason for hiding this comment

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

left comments. link of deployment: https://editbutton.dauyvuo7dvzau.amplifyapp.com/

this will work once you resolve some type errors

suryabulusu
suryabulusu reviewed Feb 14, 2024
View reviewed changes
src/pages/api/PostFormData.ts Outdated
cms_test_id:
// "https://cms.peerlearning.com/chapter_tests/655df9a23562d97a6300b53e",
test.cmsId,
test.cmsId, //x
Copy link
Contributor

Choose a reason for hiding this comment

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

remove //x

suryabulusu
suryabulusu reviewed Feb 14, 2024
View reviewed changes
src/pages/api/PostFormData.ts Outdated
test_takers_count: student.testTakers,
has_synced_to_bq: false,
optional_limits: test.optionalLimit,
marking_scheme: test.markingScheme,
test_type: test.type,
shortened_link: "",
report_link: "",
date_created: new Date().toISOString().split("T")[0],
date_created: new Date().toISOString().split("T")[0], //x
Copy link
Contributor

Choose a reason for hiding this comment

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

remove //x

suryabulusu
suryabulusu reviewed Feb 14, 2024
View reviewed changes
src/pages/api/PostFormData.ts Outdated
...(test.markingScheme ? { marking_scheme: test.markingScheme } : {}),
...(test.type ? { test_type: test.type } : {}),
...(test.shortened_link ? { test_type: test.shortened_link } : {}),
date_created: timeline.date_created,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is timeline.date_created ? date_created is in metadata

@suryabulusu suryabulusu merged commit d8df1d8 into main Feb 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suryabulusu suryabulusu suryabulusu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Prakhar-commits @suryabulusu